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

Prevent Admin Bar Items From Wrapping if There's More Items Than the Available Adminbar Width #1082

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KZeni
Copy link

@KZeni KZeni commented Mar 9, 2021

Trac ticket: https://core.trac.wordpress.org/ticket/28983 (has been open for some time without being really followed up on so hopefully this PR breathes some life back into this.)

The Trac ticket has a bunch of additional details regarding the issue being resolved here, screenshots, etc.

Code here is from version 1.0.3 of https://wordpress.org/plugins/admin-bar-wrap-fix/ (June 22, 2020; more recent releases haven't actually changed the CSS involved here) per that plugin ideally not being necessary to prevent issues that this resolves (with WordPress just preventing it from the start.) Somewhat of a "patch plugin" equivalent to a "feature plugin" WordPress has for forthcoming features, and it might be good to include it in the WP core at some point (if not now).

…Available Adminbar Width

Code from version 1.0.3 of https://wordpress.org/plugins/admin-bar-wrap-fix/ per that plugin ideally not being necessary to prevent issues that this resolves (with WordPress just preventing it from the start.)
@KZeni
Copy link
Author

KZeni commented Mar 9, 2021

I have closed #352 per the ticket associated with that (https://core.trac.wordpress.org/ticket/44438) was seen as a duplicate of an older ticket (https://core.trac.wordpress.org/ticket/28983; what's referenced above), and so this is just migrating the PR over to the active trac ticket.

@@ -138,6 +138,11 @@ html:lang(he-il) .rtl #wpadminbar * {
padding: 0 8px 0 7px;
}

#wpadminbar .quicklinks > ul > li,
Copy link
Author

Choose a reason for hiding this comment

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

I could see this being an optional include as this was simply proposed at https://wordpress.org/support/topic/awesome-plugin-works-great-13/ while it then didn't seem to be harmful & seemed to help at least one person out there.

@@ -719,6 +724,50 @@ html:lang(he-il) .rtl #wpadminbar * {
box-shadow: 0 0 2px 2px rgba(0, 0, 0, 0.6);
}

@media only screen and (min-width: 1061px) {
Copy link
Author

Choose a reason for hiding this comment

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

I could see this being an optional include as this was simply proposed at https://wordpress.org/support/topic/awesome-plugin-works-great-13/ while it then didn't seem to be harmful & seemed to help at least one person out there.

text-overflow: clip;
}
}
@media screen and (min-width: 782px) {
Copy link
Author

Choose a reason for hiding this comment

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

This is really the heart of this update. Utilize flexbox on the admin bar where it allows plugins to add items to it so that it never overflows onto a new line (instead, scaling proportionally and using an ellipses-based text overflow when needed & looking effectively the same as before when not needed.)

@ryelle
Copy link
Contributor

ryelle commented Mar 25, 2021

I'm attaching some screenshots of the admin & frontend, with a fake plugin that adds 3 menu items. I also changed my user's display name & site title to be longer, so you can see on the frontend, that even without any extra menu items the menu wraps. These screenshots were taken on Mac/Firefox at 783px wide, which is the narrowest before the menu drops to icons-only.

I think the 100px sizing introduced here is a bit too small for items to be clearly understood, so I also tried testing with only the code in the @media screen and (min-width: 782px) { … } media query.

Click screenshots for larger view 🙂

Current (trunk) With PR applied With only media query
WP Dashboard (no plugins) before-be-no-plugin pr-be-no-plugin tweaked-be-no-plugin
WP Dashboard (with plugin-added items) before-be-plugin pr-be-plugin tweaked-be-plugin
Frontend before-fe pr-fe tweaked-fe

@KZeni
Copy link
Author

KZeni commented Mar 25, 2021

That's some really nice testing there, thanks! Do you happen to have that fake plugin available for download so I can give it a shot? I can see that being helpful. 🙂

I can definitely see excluding the 2 other code blocks that aren't the primary @media screen and (min-width: 782px) { block.

Those 2 additions were from a user of the plugin having said they had used that made things nicer for them (as noted with the inline comments for the previous commit). However, I can see that there are negatives to what they've implemented in common situations & probably isn't the best option for everyone. Honestly, that code update might've been something subjective that was just a refinement (didn't address an active issue or anything serious like that) for their specific site so I'm going to consider updating the PR (and my plugin-as-a-patch plugin) to exclude that.

At that point, if we just go with the code from the @media screen and (min-width: 782px) { block, does that actually just take care of it? Does anyone see it needing any adjustments after that point, or is it just a matter of testing at various sizes & with a varying number of items in the admin bar and acting accordingly (moving forward with official adoption if no issues remain)?

@KZeni
Copy link
Author

KZeni commented Mar 25, 2021

Alright, I've updated the PR since it really seems those other 2 code blocks were more problematic than they were helpful (https://wordpress.org/plugins/admin-bar-wrap-fix/ has been updated to match as well.)

@ryelle
Copy link
Contributor

ryelle commented Mar 25, 2021

Do you happen to have that fake plugin available for download so I can give it a shot?

Sure - it's just something I put together to test this, but you can find it here.

At that point, if we just go with the code from the @media screen and (min-width: 782px) { block, does that actually just take care of it?

I also noticed a few alignment issues: the "1" here is right at the edge, and the w in New is cut off. Also, the dropdown menu is much more narrow - it looks like it matches the width of the parent item, so I'm not sure what it would do with a long submenu item.

Screen Shot 2021-03-25 at 12 16 54 PM

That said, I'd like the design team to chime in too, in case they have some other specific suggestions.

@paaljoachim
Copy link

paaljoachim commented May 11, 2021

A summary (To make it easier for someone to quickly go in and take a look.)

At current very long names look like this (with no plugins added):
Current-WP

It can cause problems on smaller screens.


I am adding in the three images that @ryelle Kelly included above #1082 (comment) in relation to changes made by the PR.
This is so that we can get a quick overview of what the PR does.

Truncation happens with very long names.

PR applied no plugins included:
PR-applied-1

PR applied plugins included:
PR-applied-includes-plugins

PR applied frontend:
PR-applied-Frontend

Adding a truncation seems like the way to go. As it would keep the admin bar in order also when long names are included.

@KZeni
Copy link
Author

KZeni commented May 11, 2021

@ryelle It's kinda just relying on the browser's overflow:hidden; & text-overflow:ellipses; for that supplied content & element dimension (I'm guessing the browser considered the content that's there is smaller or effectively the same as an ellipses so it just shows the content). Also, the sizing is pretty much handled by the browser as well based on the content and display:flex;.

At that point, I'm not really sure how we could change the PR or otherwise refine specific elements like that having it rub up against another item (forcing it to have a gap between the items would leave less room for the content we care more about at that point when this really is just coming into play as a bunch of items are being shown in the menu [already a bit complicated at that point & possibly best to show the small bit of content in the menu item rather than nothing/spacing.])

@paaljoachim You can see I updated things per 0fa39b6 on 3/25 to help remove the unnecessary truncation when there's enough room to show the menu items & their content in the menu bar.

As such, the screenshots you've included by pulling from earlier in that day (before those edits were made & were actually what spurred the edit to be made per the unnecessary truncation shown in those screenshots) are actually a bit out-of-date in terms of what it looks like with the current PR.

@paaljoachim
Copy link

paaljoachim commented May 11, 2021

The concept of truncating very long words when it is needed feels like the way to go.

EDIT: There seems to be a minimum spacing needed to show the information that should not be truncated.

Mobile admin bar is now accounted for as plugins have recently been adding their own items to the mobile menu as well & potentially causing items wrap onto a new line outside of the admin bar given the available space. Instead of wrapping to overlap the actual content, it simply offers a horizontal scroll for when there are too many items to show on screen at once (keeping the items all within the admin bar's one row of icons.)
#wpadminbar {
display: flex;
flex-wrap: nowrap;
padding-bottom: 15px;
Copy link
Author

Choose a reason for hiding this comment

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

15px beign the browser scrollbar dimension per https://codepen.io/sambible/post/browser-scrollbar-widths (with the overflow-y:hidden; handling the difference between them)

@KZeni
Copy link
Author

KZeni commented Jun 1, 2021

Updated PR to match version 1.2 of Admin Bar Wrap Fix plugin (plugin as a patch): https://wordpress.org/plugins/admin-bar-wrap-fix/

It now addresses mobile admin bar as it seems plugins are more likely to add items to the mobile admin bar than before (now causing the items to wrap and overlap the content area of the page [instead of offering a horizontal scroll as needed as this patch implements.])

@KZeni
Copy link
Author

KZeni commented Jun 1, 2021

I personally saw Yoast SEO, W3TC, Gravity Forms, and Monster Insights all having menu items on mobile. Definitely starts wrapping on mobile alongside the standard set of WordPress items (with it being reasonable for that set of items being present [or more/similar.])

@KZeni
Copy link
Author

KZeni commented Oct 31, 2022

Well... it's been more than a year now since the latest message regarding this topic (about 8 years since the issue was first reported via https://core.trac.wordpress.org/ticket/28983) and this hasn't been formally addressed yet with WP 6.1 around the corner.

Maybe this is something that can be considered for WP 6.2.x? It doesn't need to exactly implement this solution (since this being plugin-ified [per https://wordpress.org/plugins/admin-bar-wrap-fix/] has it not being as optimized as it would be when natively implemented), but I'd like to see WP be able to handle the additional admin bar items that plugins may add better than it does now (wraps onto a new line that then overlaps the content of the site, etc.)

@paaljoachim
Copy link

I posted about this PR in the Core Editor Slack channel. As well as added a comment to the agenda for todays core editor meeting. https://make.wordpress.org/core/2022/11/07/editor-chat-agenda-9-november-2022/#respond

@KZeni
Copy link
Author

KZeni commented Nov 9, 2022

@paaljoachim Would making an adjustment to the admin bar be considered part of the Core Editor? Isn't there a Core UI, Design, Accessibility, etc. option that would be potentially more applicable here? I figured the core editor setup is more about the block/gutenberg editor while this is more about a core UI element of WordPress as a whole and an accessibility & visual issue that may occur in certain (not exactly uncommon) circumstances.

I'm glad to hear it's being brought up & discussed nonetheless, though!

@paaljoachim
Copy link

My mistake.
I went ahead and added it to the Core slack channel as well.
https://wordpress.slack.com/archives/C02RQBWTW/p1668002642964459

If you happen to be around and would like to attend the Core Dev chat. There is some more info here: https://make.wordpress.org/core/

@strarsis
Copy link

strarsis commented Mar 1, 2023

@KZeni: Thank you for all the work on this! I just encountered this issue and it can disrupt the design on narrow viewports.

I hope this is merged into core soon!

@KZeni
Copy link
Author

KZeni commented Mar 1, 2023

@strarsis:

@KZeni: Thank you for all the work on this! I just encountered this issue and it can disrupt the design on narrow viewports.

I hope this is merged into core soon!

I definitely agree with it being implemented as part of the WordPress core. That's why it’s been submitted to the WordPress github repo (you are here now) as a pull request & core trac ticket. Hopefully, it sees more attention/consideration to get officially adopted and/or it’s just fixed via other means (ex. a larger overhaul of the admin bar) at some point, but I can’t say when that might be as I’ve checked in on it every so often with it not really getting much traction for one reason or another. I suppose that situation validates this being a plugin as it’s been useful for years now due to the core issue still not being addressed (I’ll then look to update the plugin to make it auto-deactivate [or otherwise make it do nothing & show a notice of this fact] once the WordPress core addresses this formally by having it check the site’s current WP version to see if it’s needed or not so it should be safe to have this on sites [while it’d definitely be ideal to not need this plugin at some point in the future.])

@ajtruckle
Copy link

I have just installed the plugin. Why hasn't it been applied yet in 6.x?

@KZeni
Copy link
Author

KZeni commented Jul 18, 2023

@ajtruckle Good question. It's been hanging out as a core ticket reporting this issue for 9 years now(!?) per https://core.trac.wordpress.org/ticket/28983 with me having implemented this fix with my feature-as-a-plugin patch-as-a-plugin about 2 or so years ago while then also reporting my fix with a proposed patched via the core tickets & also this GitHub repo. It was brought up by others a few times but never got the attention needed to approve it for a WP version release.

I think the main thing is getting the attention for this to then be included as standard in the WP core, but I've been busy with other things lately while the plugin has met my own needs for this in the meantime. So, when the interest of others to get it included went away a few times over this period (probably due to not getting the attention of the correct people that are more involved with approving & including patches in future releases) I ended up leaving things as-is.

That said, my fix appears to be just as valid today as it was back then (probably just making a few accommodations to address the merge block and/or any other aspect that might've changed that would've affected the proposed patch to make it more readily available to be merged into the core's upcoming version.) As such, anyone/everyone is more than welcome to take to the Making WordPress Slack, core trac ticket, etc. to help carry this along into being officially adopted. Also, improvements to my plugin (to then be proposed as the core WP patch) are welcome via https://github.com/KZeni/Admin-Bar-Wrap-Fix & the plugin's support forum (https://wordpress.org/support/plugin/admin-bar-wrap-fix/.)

Meanwhile, I've recently been seeing discussions regarding Phase 3 of the WordPress roadmap (https://wordpress.org/about/roadmap/) as having a redesigned site admin experience (with the full site editor & some select setups/plugins giving a glimpse of what that design might be like) which may then just replace the admin bar with a new implementation which should then formally address that issue of the old admin bar (likely by just having it built to not have that shortcoming.) However, that might be a while off as that seems to be more in the discussion & prototyping stage with the implementation of this potentially being rather involved. So, again, I'd be happy if someone that has the time to push this fix into being officially adopted for the time being.

@ajtruckle
Copy link

ajtruckle commented Jul 19, 2023 via email

@KZeni
Copy link
Author

KZeni commented Jul 19, 2023

@ajtruckle Ah, that’s unfortunately a mixture of a browser limitation & the want to keep this solution more simple than not.

I needed to introduce overflow-x to make it so the mobile menu items could be scrolled to view the additional items that would otherwise spill over, and that unfortunately brings overflow-y into the mix per the current browser behavior where it was then better to use overflow-y:hidden; rather than trying to show the dropdown which then has it be caught by the browser and forced to also show via scroll (as far as I know, there is no way to have browsers allow that horizontal scrolling via native scrolling while then also having that same element have items that extend outside of the scrollable area that also scroll with it.)

To avoid that, we’d probably need to introduce some JavaScript to handle things and/or restructure parts of the menu itself (ex. Do we have the drop downs cloned to be outside of the scrollable area while JS keeps them aligned with their menu item & toggles them accordingly to make it appear like one might expect given existing functionality? Do the items that go outside of the current viewport be placed in a “…” menu at the end with those other items moved to be offered inside of that via JS so there’s no scrolling & the drop downs can just show normally at that point? Do we look to resize the buttons/icons to specifically fit based on the current viewport & number of items needing to show in the admin bar? Do we just have the admin menu be presented as a simple button where it then toggles something like a full-screen overlay that’s specifically designed for a dynamic number of items with any number of sub-items on a mobile screen like a collapsible/navigable vertical menu? There are probably other options that could be considered, too.)

Meanwhile, this approach that’s much simpler than all of those options (no JavaScript or changing of the menu itself; just introduce a CSS approach that better utilizes more modern layout options given the existing overall style & content) still makes it where one can certainly just navigate to the top-level admin bar item on mobile where that resulting page then offers what would otherwise be shown in the dropdown. So it does add one more page load, but it’s only on mobile and also keeps the overall proposed patch & implementation much simpler (still seems like an overall improvement compared to not having it even when that one particular & situational convenience is lost along the way.)

Otherwise, I’m definitely open to input both in code commits & ideas of what might be the best way to keep it as simple as possible (more performant, less likely to break after an update, etc.) while being fully accommodating and sticking to WordPress’ core design direction, flexibility, and code practices.

Honestly, at a certain point this discussion could evolve into being about larger topic of the WordPress roadmap’s phase 3 with the WP core potentially getting a new admin bar to coincide with the upcoming admin redesigns. That could then correct the shortcomings of the current admin bar while meshing well with the admin redesign without needing any of the patches or plugins being discussed here. Again, I’m open to input on any & all of this. I just don’t want to spend too much time making a more complex solution to address that one missing convenience on mobile when the way it ends up working to address that might not be liked by some people due to potentially diverging too far from the native design/behavior and/or adding more complexity than they like, etc. when the thing it’s addressing might be getting officially overhauled in a future update that’s currently being discussed & worked on (which that might as well be the thing to discuss & work on instead.) Also, there technically are plugins that do that more overhaul approach and/or let you show/hide items from the menu to address it that way. This approach is currently just the most simple & straightforward way that helps patch a layout issue the menu can sometimes have (not shooting down future ideas & contributions regarding this or anything like that; just sharing how this currently looks to fit in with things.)

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.

5 participants