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

Polish admin bar display now that it's using SVGs. #6488

Closed
indigoxela opened this issue Apr 29, 2024 · 27 comments · Fixed by backdrop/backdrop#4728
Closed

Polish admin bar display now that it's using SVGs. #6488

indigoxela opened this issue Apr 29, 2024 · 27 comments · Fixed by backdrop/backdrop#4728

Comments

@indigoxela
Copy link
Member

indigoxela commented Apr 29, 2024

Description of the task

As of #364, Backdrop's admin bar now uses inline SVG, sized by em. 🚀
This is a follow-up to discuss and possibly fix the design / UX aspects (like margin).

Currently there seems to be some wasted space between the menu items, which will have impact on display on narrower screens.
And the admin bar's slightly higher, too, which may affect custom themes (for example, if they have a fixed menu bar).

For comparison:

wasted-space

@klonos
Copy link
Member

klonos commented May 2, 2024

Rehashing what I said in #6495 (which I'll be closing as a duplicate of this issue here shortly)...

With #364, the admin bar icons have increased size, which is good for UX (larger area to interact with is better on touch-enabled devices), however there is now too much left/right padding as well (1.5em and 1em respectively). That causes the the following:

  • icons appear too far from each other visually (this is just aesthetics and subjective opinion)
  • increase of mouse "travel time" when moving horizontally across the admin bar
  • decrease of the width that causes reaching the breakpoint that switches the admin to mobile version

Some screenshots for visual:

  • Before:
    image
  • Now (1.5em left and 1em right padding):
    image
  • Proposed (1em and .5em):
    image

Perhaps we should decrease the padding even further(?)

@indigoxela
Copy link
Member Author

@klonos I'd suggest to start with a pull request. It's always easier to decide, if people can look at something "real" on their devices with their familiar screen resolution (in the sandbox). 😉

@quicksketch quicksketch added this to the 1.28.0 milestone May 2, 2024
@klonos
Copy link
Member

klonos commented May 2, 2024

@argiepiano this is the issue you are looking for 🙂

@indigoxela
Copy link
Member Author

indigoxela commented May 3, 2024

Here we go, a PR is available for testing and review.

It reduces the width of menu items. Although it's still wider than the old one, it's more compact now.

It also reduces the height of the admin bar to (almost) fit the previous height, to reduce the impact on existing themes, custom or contrib, that use a fixed navigation bar on their own (Scenery, for example).

@klonos
Copy link
Member

klonos commented May 3, 2024

@indigoxela I wanted to quickly say that I like it 👍🏼 ...I'll return with more useful feedback.

@klonos
Copy link
Member

klonos commented May 3, 2024

...here's a quick thought though: could we be adding the height of the admin bar dynamically into a data-* attribute, in a way that would allow themes to also dynamically adjust their offsets as the admin bar height increases/decreases? (follow-up - just a thought that I wanted to document somewhere quickly)

@indigoxela
Copy link
Member Author

... could we be adding the height of the admin bar dynamically into a data-* attribute...

Did you mean a CSS variable? (Instead of data attrib) That might be helpful for theming. No idea, if that's feasible. We could explore it in a follow-up.

@argiepiano
Copy link

I like it! Thanks @indigoxela! I'll let subthemers and UI experts opine. To me, it looks good ;)

@izmeez
Copy link

izmeez commented May 3, 2024

I like this too.

@quicksketch
Copy link
Member

in a way that would allow themes to also dynamically adjust their offsets as the admin bar height increases/decreases? (follow-up - just a thought that I wanted to document somewhere quickly)

Themes already don't need to do anything to adjust the admin bar height. The body margin (or border actually) is adjusted by admin_bar.css. The only thing they need to not do is override the provided border:

.admin-bar body {
  border-top: 2.25em solid #2D2D2D !important;
}

That said, we could use set a variable to let themes know how tall the admin_bar is.

I sort of liked the larger admin bar height but I can accept this compromise that we retained the larger font size while keeping the height the same.

@klonos
Copy link
Member

klonos commented May 4, 2024

That said, we could use set a variable to let themes know how tall the admin_bar is.

Yes. That. But can be a follow-up, as @indigoxela said.

@indigoxela
Copy link
Member Author

Themes already don't need to do anything to adjust the admin bar height.

@quicksketch not the height, that's right, but if themes (or modules) place something (also positioned) right under the admin bar, they do have to consider the actual admin bar height for that. That affects one of my contrib themes and may affect custom themes, too. That's why I reduced the height, so the impact of the change is less likely to cause visual glitches.

@quicksketch
Copy link
Member

@indigoxela Would using a CSS property help to avoid this problem in the future maybe? I filed a PR against yours to see if that would be a good idea: indigoxela/backdrop#6

The icon system is entirely dependent on CSS properties, so maybe we should lean into them and expose things like this that would be particularly helpful for themes and modules. Both var() and calc() are "baseline" CSS capabilities of all browsers at this point (fully supported since 2017) according to caniuse.com.

@indigoxela
Copy link
Member Author

indigoxela commented May 4, 2024

Would using a CSS property help to avoid this problem in the future maybe?

It might. 🤔 What I particularly like about your suggestion (PR) is, that we switch from a "magic number" to a named var. Good for themers, but also good for our future selves (whenever we have to change the value again). 😉 Will play with it locally.

@indigoxela
Copy link
Member Author

indigoxela commented May 6, 2024

Based on the suggestion by @quicksketch we're now using the property --admin-bar-height to make it easier for themes to use this like: top: var(--admin-bar-height); to automatically get the right value for positioning something right below the admin bar.
Visually, there's absolutely no change, just an easier access to the height and no more need to remember the px value, or update it (in your theme), if the value changes later.

Thoughts?

@izmeez
Copy link

izmeez commented May 7, 2024

I have tested the latest code in the PR locally as a patch against 1.28.0-preview. It works great. It is such a relief the have the admin bar back where it fits on the display better. It would be nice to see this get into the release.

@izmeez
Copy link

izmeez commented May 7, 2024

My previous comment was premature. Further testing the --admin-bar-height and increasing it does not adjust the whole admin bar display but instead adds a extra zone of depth. The contrast is not great but it is possible to discern a less black horizontal bar.
bd-issue-6488-adminbar-polish-screenshot

@indigoxela
Copy link
Member Author

indigoxela commented May 7, 2024

My previous comment was premature. Further testing the --admin-bar-height and increasing it does not adjust the whole admin bar display

@izmeez Sorry, but it's not supposed to do that. Why are you assuming, it would? It's a value, that's supposed to be informational only. Not to adjust it in themes.

And as adjusting via single var isn't actually in the scope of this issue here - should I revert this change? It seems to be confusing.

@izmeez
Copy link

izmeez commented May 7, 2024

I changed the value --admin-bar-height in admin-bar.css. I guess I misunderstood. When I change the font size in #admin-bar it works as expected. Yes, I see the comment. I just got thrown off.

@izmeez
Copy link

izmeez commented May 7, 2024

Would a comment above --admin-bar-height help?
Property --admin-bar-height is for themes to use like: top: var(--admin-bar-height); to automatically get the right value for positioning something right below the admin bar.

@olafgrabienski
Copy link

I've applied the PR on a test site. It works really good for me in getting a lot of important space in the admin bar back.

For comparison I'm listing how wide a screen must be to display the different admin bar modes. Tested with English interface and with all components (search bar, language switcher, etc.) enabled:

Full menu (depends on username length)

  • 1.28 pre: 1786px
  • with PR: 1559px

Normal menu + More tasks

  • 1.28 pre: 1238px
  • with PR: 1086px

Hamburger menu + More tasks

  • 1.28 pre: below 1238px
  • with PR: below 1086px

@indigoxela
Copy link
Member Author

@olafgrabienski many thanks for also testing, and for review. I added the leading zeroes.

@izmeez
Copy link

izmeez commented May 8, 2024

This comment is clearly beyond the scope of this issue. However, it got me thinking: now that the admin bar with SVG icons can change size by simply changing the font size, what about having something that can be changed in the settings.php file like, $conf['admin-bar-font-size'] = 2.35; so it can be increased or decreased for those who prefer?

@indigoxela
Copy link
Member Author

This comment is clearly beyond the scope...

😂 oh, really? 😉

Via settings.php, via admin UI, leave it to contrib... a lot to discuss, which should better be done in a different issue. At least, that's what I'd suggest.

@quicksketch
Copy link
Member

Code-wise and visually this looks good to me. IMO it's a little tight but multiple people have said that the admin bar collapsing earlier for them is not desirable. It's a good compromise to keep the font size at 12px but tighten up the spacing.

@indigoxela
Copy link
Member Author

IMO it's a little tight...

Yes, it's a compromise to avoid the "premature hamburgerization" 😉 of the menus in the admin bar. Design-wise the initial change looked very good (nice distinction between items), but... 🤷

@quicksketch I commited your requested comment change.

@quicksketch
Copy link
Member

Thanks @indigoxela! I merged backdrop/backdrop#4728 into 1.x for 1.28.0. We can revise again if we have the will for further adjustment. Thank you @herbdool, @olafgrabienski, @izmeez, and @klonos for your feedback and review!

@jenlampton jenlampton changed the title Follow up: Polish admin bar display, now that it's using SVG Polish admin bar display now that it's using SVGs. May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants