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

fix: light scrollbars and line height #4826

Closed
wants to merge 1 commit into from
Closed

fix: light scrollbars and line height #4826

wants to merge 1 commit into from

Conversation

deboer-tim
Copy link
Collaborator

What does this PR do?

After the PatternFly removal I noticed that the scrollbars on Mac are in light-mode. PF-dark was setting the color-scheme before, setting it again fixes the problem and means we can also remove the explicit text-white. (maybe some of the changes in #4803 too?)

PF was also setting the line height to 0.9. I'm not tied to it, but it makes buttons, multi-line text, and preferences nav items all less tall. Maybe I'm just used to it, but I prefer a smaller window and 0.95 gets most of the way there & uses less space.

Screenshot/screencast of this PR

1.5.3:
Screenshot 2023-11-15 at 6 55 43 PM

Post PatternFly:
Screenshot 2023-11-15 at 4 15 15 PM

This PR:
Screenshot 2023-11-15 at 4 14 40 PM

What issues does this PR fix or reference?

N/A

How to test this PR?

Open dashboard, preferences.

After the PatternFly removal I noticed that the scrollbars on Mac are in light-mode.
PF-dark was setting the color-scheme before, setting it again fixes the problem and
means we can also remove the explicit text-white. (maybe some of the changes in
#4803 too?)

PF was also setting the line height to 0.9. I'm not tied to it, but it makes buttons,
multi-line text, and preferences nav items all less tall. Maybe I'm just used to it,
but I prefer a smaller window and 0.95 gets most of the way there & uses less space.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim deboer-tim requested review from benoitf and a team as code owners November 15, 2023 23:57
@deboer-tim deboer-tim requested review from jeffmaury and feloy and removed request for a team November 15, 2023 23:57
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM on Windows

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

I would not opt-in for this change (color-scheme), it will delay the work to have proper light/dark theme.

For the line-height, I would not follow css approach as well, it's breaking the tailwind flow
as elements are interconnected. It's making harder to have clean stuff.

@deboer-tim
Copy link
Collaborator Author

color-scheme sets the defaults (light=white background, black text, scrollbars colors, etc, dark=black background, white text, etc). I was assuming we'd set it now to fix scrollbars (and anything else like text-white where the default is fine), then future light mode would toggle it. What is the other option? I don't think we want to custom style scrollbars or to use text-black dark:text-white in the future when those could already be the default colors.

AFAIK Tailwind doesn't have an equivalent option to globally scale line height. Do you think we should override all the font sizes in config, change scaling/padding in all the affected components individually, or something else?

@deboer-tim
Copy link
Collaborator Author

Light mode fixed via #4910. I will submit separate issues and/or PRs for components affected by change in line height.

@deboer-tim deboer-tim closed this Nov 22, 2023
@deboer-tim deboer-tim deleted the dark branch November 30, 2023 20:13
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.

None yet

3 participants