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

Introduce a CSS variable for the toolbar height (bug 1171799) #18518

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

timvandermeij
Copy link
Contributor

This refactoring lays the foundation for making the toolbar height configurable in Firefox via the browser.uidensity preference. For this to work correctly the toolbar height must be defined in a single place that can easily be updated dynamically, hence this patch which moves it to a CSS variable in such a way that the rest of the UI adapts correctly if the value is changed.

Extracts a part of #18385.

This refactoring lays the foundation for making the toolbar height
configurable in Firefox via the `browser.uidensity` preference. For
this to work correctly the toolbar height must be defined in a single
place that can easily be updated dynamically, hence this patch which
moves it to a CSS variable in such a way that the rest of the UI adapts
correctly if the value is changed.

Co-authored-by: Calixte Denizet <calixte.denizet@gmail.com>
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Jul 30, 2024

Note that this patch deliberately doesn't resize the inner toolbar elements if the toolbar height is changed yet: that'll be part of a next step because it requires more changes. Not only will those be easier to do once this is in place first, but in the preview in #18385 I found that the sidebar didn't resize correctly if the toolbar height was changed, so that's another reason I wanted to keep this step small so it's more manageable and easier to review.

This is a screenshot the preview of #18385 when the toolbar height CSS variable is set to 50px (note that the sidebar overlaps the toolbar):

Before

This is a screenshot of this patch when the toolbar height CSS variable is set to 50px (note that there is no overlap):

After

You can change the toolbar height dynamically now by opening the preview and changing the --toolbar-height CSS variable via the developer tools (using inspect element) to see how the UI responds.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/f8b26890517ba2a/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/f8b26890517ba2a/output.txt

Total script time: 1.04 mins

Published

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thank you!

(Splitting that huge PR into more manageable chunks is very helpful, since I don't think that we can really review it otherwise; and as mentioned in #18385 (comment) there seems to be lots of small movement which might be difficult to pin down in a huge patch.)

@timvandermeij timvandermeij merged commit 0269cf5 into mozilla:master Aug 1, 2024
6 checks passed
@timvandermeij timvandermeij deleted the viewer-toolbar-height branch August 1, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants