Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Focus View (GitHub only) #28

Merged
merged 13 commits into from
Apr 16, 2024
Merged

Focus View (GitHub only) #28

merged 13 commits into from
Apr 16, 2024

Conversation

jdgarcia
Copy link
Contributor

The signed out and user row (when signed in) UIs have been redone

Focus View only shows up if you have GitHub connected, otherwise it's just a blank screen (for now, next PR will be "connect an integration" UI).

Copy link
Collaborator

@miggy-e miggy-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a couple of small things. I also would like it if the focus results were cached, but this can be handled in a separate issue

src/popup/components/FocusView.tsx Show resolved Hide resolved
src/background.ts Show resolved Hide resolved
Copy link
Contributor

@gitkrakel gitkrakel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout on Firefox ESR (115.x) is completely broken

Sign-in page

Screenshot from 2024-04-15 10-25-01

Focus View

Screenshot from 2024-04-15 10-31-54

(optional) Nits with layout when permissions are requested

Scrollbar in Focus View when permissions are requested

Screenshot from 2024-04-15 10-35-20

Unnecessary word wrap when cloud and self-hosted are both requested

Screenshot from 2024-04-15 10-36-40

Other

yarn lint has some warnings you might want to fix.

@jdgarcia
Copy link
Contributor Author

@gitkrakel well, the good news is that the layout is broken simply because Firefox ESR doesn't support nested CSS. The bad news is I can't use nested CSS which I was happy to be able to use 😢

Scrollbar in Focus View when permissions are requested

That's intentional. The popup has a max height enforced by the browser, and I opted to make the permission banner and the user bar static, and everything else scrollable.

Unnecessary word wrap when cloud and self-hosted are both requested

That's a consequence of the font size and weight used in the Figma design.

@jdgarcia jdgarcia requested a review from gitkrakel April 16, 2024 16:51
@jdgarcia
Copy link
Contributor Author

jdgarcia commented Apr 16, 2024

@gitkrakel I fixed the Firefox ESR issues. I'll ask Bryce about the permission banner word wrap but that might not get updated til the next PR.

@gitkrakel
Copy link
Contributor

gitkrakel commented Apr 16, 2024

@jdgarcia So I looked into CSS nesting more, and a couple notes:

  1. The current Firefox ESR does in fact support nesting, but it's disabled by default. The user needs to go into about:config -> layout.css.nesting.enabled to toggle it on, and then the Focus View seems to work as intended:

image

  1. CSS nesting is enabled by default starting in Firefox 117: https://www.mozilla.org/en-US/firefox/117.0/releasenotes/. Therefore, regular releases of Firefox already support it
  2. The next Firefox ESR is 128.0, which comes out in July 2024 (https://whattrainisitnow.com/calendar/).

So if CSS nesting is something you feel strongly about, I think it's fine to compromise and tell Firefox ESR 115 users to enable the about:config flag (they should be a small fraction of the userbase anyway)

Copy link
Contributor

@gitkrakel gitkrakel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good now 👍

@jdgarcia jdgarcia force-pushed the github-focus-view branch from ae790c9 to 83b825d Compare April 16, 2024 20:49
@jdgarcia jdgarcia merged commit 43b49c0 into main Apr 16, 2024
1 check passed
@jdgarcia jdgarcia deleted the github-focus-view branch April 16, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants