Closed klonos closed 5 years ago
@klonos I'm stealing this issue, and re-purposing it to simply add icons into admin bar.
I don't see a reason to add an admin UI in core, that can be added in contrib. We'll just do this (and get it as close to "right" as possible) so most people won't need to worry about changing it.
Previous comment from @klonos (so it won't be lost!) follows:
[moved to #3819]
Looks great! I like it.
Thanks so much @jenlampton 🙏 ...looks awesome 👍 ❤️
Comments:
I cannot reproduce this now, but when I first logged in the PR sandbox, the home menu item had only text and no icon. Clearing caches fixed things. Anyway...
I was slightly skeptical about the use of the path as class, but I now think that it is perfectly OK, because we will be able to differentiate possible same labels in different menu items.
An alternative to consider though would be to use a series of classes per path segment, so we could avoid long classes (like a single .admin-config-development-configuration-sync-diff
class), plus we would be able to do this:
.admin.config
.admin.config.development
.admin.config.development.configuration
.admin.config.development.configuration.sync
.admin.config.development.configuration.sync.diff
There is no CSS class being added to last/deepest children menu items.
We should use similar filenames for the icons as in https://github.com/backdrop/backdrop/tree/1.x/core/themes/seven/images (specifying color, size, and fontawesome.com name).
The CSS in the PR repeats things like background-size: 16px;
and padding-left: 29px;
for all icons. Can we please add these in a generic selector?
The current icon for the user counter looks dated. We should replace it with https://fontawesome.com/icons/user-friends
We should add https://fontawesome.com/icons/sign-out-alt or https://fontawesome.com/icons/door-open as an icon for the "Log out" menu item.
Replace the single-person icon for "User accounts" with https://fontawesome.com/icons/users
Use the single-person icon for the user profile menu item. Alternatives: https://fontawesome.com/icons/address-card or https://fontawesome.com/icons/user-circle (I personally like the later).
I like the "control panel" icon that WordPress uses for their "Settings" menu. Perhaps use https://fontawesome.com/icons/sliders-h, but rotate it 90° to be vertical.
I don't mind the paintbrush for the "Appearance" menu, but perhaps https://fontawesome.com/icons/palette would make more sense now that we have improved color.module support (the palette color "wells" resemble the new round color selector fields we now use 😄).
WordPress uses https://fontawesome.com/icons/plug for the plugins/modules menu item. Not to say that the puzzle piece is not cool; I personally like both equally.
I don't like the icon used for "Structure" (I like the one they use in Drupal better; but still not even remotely ideal). I wish that fontawesome.com had something like a blueprint icon (like https://www.flaticon.com/free-icon/blueprint_182657), but the only possible alternative I could find was https://fontawesome.com/icons/pencil-ruler 🤔
In terms of feedback on specific icons, I would have been fine with them the way they are in the PR. After reading @klonos feedback, I thought I'd chime in:
1) Content = Pencil is great 2) User accounts = is good. Multi person icon is better 3) Appearance = I like the paint brush. I do not think the Palette would be an improvement. 4) Functionality = Puzzle Piece works for me, but I see a persuasive argument in favor of the plug (users seem to get the concept of plugins more than modules, thanks to Wordpress). 5) Structure = I like the current PR icon better than the suggested alternatives 6) Configuration = Current icon in PR is easily understood and commonly used. 7) Reports = Boring but effective 8) User Counter = I agree that this icon is dated and could use a refresh, suggested alternative is much better and fits in better with other icons. 9) Logout = I like the door better than the other option suggested.
It's not clear to me where @klonos wants to use "control panel" icon. I would approve of the PR as it is. I only offer these opinions in case more opinions are helpful.
It's not clear to me where @klonos wants to use "control panel" icon.
For the "Configuration" menu item. The respective section for that in WP is "Settings" (see screenshot a few comments up).
...to be clear, I'd hate to derail this with bike-shading about the icons. Just suggestions 😄. My actual concerns are points 1 to 4.
...I also just noticed this now:
I also only included them on the top-level links, and only on desktop (when we're sure there's enough room for them)
I think we should be fine to use the icons on mobile too. The admin bar collapses to a single "Admin bar" drop-down menu with a single top-level item, so I don't see any reason why not have icons in the second+ level items.
I don't see any reason why not have icons in the second+ level items.
One reason not to is that choosing icons is hard (as we found here!) and adding them to the top level seemed like the most-win for the least-pain :) I did think about adding an icon to the one top-level link on mobile "Admin bar" but I couldn't decide what icon :(
1 I was slightly skeptical about the use of the path as class, but I now think that it is perfectly OK
good :)
- There is no CSS class being added to last/deepest children menu items.
good :) they should only be on the top-most links.
- We should use similar filenames
Done. Also moved these guys to admin-bar module.
- The CSS in the PR repeats things like background-size: 16px; and padding-left: 29px; for all icons. Can we please add these in a generic selector?
The background-size
declaration needs to come after the background
declaration, so the only thing we'd be saving is moving the padding-left: 29px;
which seemed not worth-it to me.
- The current icon for the user counter looks dated. We should replace it with fontawesome.com/icons/user-friends
Done.
- We should add fontawesome.com/icons/sign-out-alt or fontawesome.com/icons/door-open as an icon for the "Log out" menu item.
I prefer 'sign-out' so I've used that instead. what do you think?
- Replace the single-person icon for "User accounts" with fontawesome.com/icons/users
done.
- Use the user-circle icon for the user profile menu item.
Done.
- Perhaps use fontawesome.com/icons/sliders-h, but rotate it 90° to be vertical.
For what? Also, this is getting too crazy...
- I don't mind the paintbrush for the "Appearance" menu, but perhaps fontawesome.com/icons/palette would make more sense ..
I also prefer this because it doesn't look as much like the pencil we're using for Content
- WordPress uses fontawesome.com/icons/plug for the plugins/modules menu item.
I tried the plug (see below) but prefer the puzzle piece so I left it.
12 > I don't like the icon used for "Structure"
It was also my least favorite. I dislike pencil-ruler
because it's also too much like the pencil we use for Content. I ended up going with sort-amount-down
, which I prefer:
Thanks @jenlampton ...I love this ❤️!!
- There is no CSS class being added to last/deepest children menu items.
good :) they should only be on the top-most links.
During my testing, I have discovered that they are also being added to the children items ...just not the deepest ones.
- We should use similar filenames
Done. Also moved these guys to admin-bar module.
Thanks. You know me and my OCDs 😜 ...moving them to the admin bar module seems a correct move 👍. I was thinking that if we were to add the icons used in /admin/config
in the respective "Configuration" menu in admin bar, then we'd need to place them there too (but that's TBD and a follow-up).
- We should add fontawesome.com/icons/sign-out-alt or fontawesome.com/icons/door-open as an icon for the "Log out" menu item.
I prefer 'sign-out' so I've used that instead. what do you think?
Don't have a specific preference. ...although an open door might be interpreted as both log in and log out, so perhaps better with the "sign out" icon. It also has the name for it 😉
- Perhaps use fontawesome.com/icons/sliders-h, but rotate it 90° to be vertical.
For what? Also, this is getting too crazy...
For the "Configuration" menu. Similar to what WordPress is using for its "Settings" menu.
I tried the plug (see below) but prefer the puzzle piece so I left it.
All good 👍 ...was just mentioning alternatives.
It was also my least favorite. I dislike
pencil-ruler
because it's also too much like the pencil we use for Content. I ended up going withsort-amount-down
, which I prefer:
Yeah, neither is ideal. Lets go with that unless someone has a better alternative. Perhaps something more fitting will be added in the fontawsome list in the future.
Everything looks beautiful now ...we needed some "bling" for our bday release 😄
The PR sandbox showed no icons this time. Clearing caches once showed all icons except for the home icon ...a second cache clear showed that too.
Noting that there is left padding added to all menu items, including the non-top-level ones. Handy if we are to add icons to them in the future (which I think we should), but not if we are not to do that ever.
...thanks for posting those screenshots btw, and thanks for adding an icon for the "Development" menu. I think that:
a) we should use that icon for the "Development" section in /admin/config
instead of the wrench we are currently using.
b) that "Development" top-level menu is actually a bug 😅...it only shows up when devel.module is installed and enabled, and I think it should be merged with the menu item under "Configuration". We should either have one "Development" item in the top level, or one under "Configuration".
I just logged into sandbox and the icons were visible immediately. But, I seem to recall needing to clear the cache the very first time I tried this PR 3 days ago. I'm not too worried about this.
Looks wonderful. I give my stamp of approval, but given that I have not looked at the code, I'm not going to change the status.
Loving this ! Add a minor a11y concern in PR review.
But I can't resist the bikeshedding: The "page' icon for reports can be nice in replacement for content's "pencil". And then have a"info circle" or a "graph" icon (like D8) for reports.
And then have a"info circle" or a "graph" icon (like D8) for reports.
Yes, this is better! Added and pushed :)
The "page' icon for reports can be nice in replacement for content's "pencil".
Hm, I know @quicksketch and I both preferred the pencil. Leaving that one unless we get some more feedback that file
is better there than pencil
.
I like pencil for now. If we ever have an "Edit this page" button in the toolbar, we'll want to use the pencil for that instead. But I think we can make that adjustment if/when that happens.
agreed. let's stop the bikeshedding ;) I'd rather focus on the accessibility issue
Also there are some concerns around mobile display... it would be nice to have icons show up on mobile for the first level items. But that could be a follow-up. Thoughts?
Mobile display can be fixed: https://i.imgur.com/Sf5wxRh.png ;
As .top-level
class is added by JS only on "desktop view", we need to get rid of it in icon CSS rules.
Replace
#admin-bar #admin-bar-menu.top-level .dropdown li > a { /* padding stuff */ }
[dir="rtl"] #admin-bar #admin-bar-menu.top-level .dropdown li > a { /* padding stuff */}
#admin-bar-menu.top-level li > a.admin-XXX { /* icon definition */}
with
#admin-bar #admin-bar-menu > li > .dropdown > li > a {/* padding stuff */ } // only first level
#admin-bar-menu.top-level li > a.admin-XXX { /* icon definition */ }
One may appreciate a padding-left for 2nd level item on desktop. Sorry about the raw diff, it was just a in-browser test.
Okay:
Ready for another review: https://github.com/backdrop/backdrop/pull/2456
I just logged into sandbox and the icons were visible immediately. But, I seem to recall needing to clear the cache the very first time I tried this PR 3 days ago. >I'm not too worried about this.
That's because if there is another person that happens to log in on a fresh sandbox, and they clear caches, then there is no need for anyone else logging in after them to clear caches 😉
...
Other than that RTBC 👍
The "Admin bar" menu item goes pink on mobile 😆
Darn my debug code... Fixed now!
The icons "jump" a couple of pixels left/right when the admin bar is hovered on
That's what I get for trying to be clever... fixed and code simplified.
Looks great. Few concerns on desktop:
Close to RTBC for me <3
I have left one comment in the PR re coding standards (a missing space), plus another suggestion.
I think address those plus the missing gray background on hover for "Home" that @opi mentioned and we're RTBC 👍
One minor suggestion: why limit the adding of classes to only the top-level menu items? If we added them to all, we'd be making it easier for people to add icons via their custom themes, plus would be one less conditional in the code 😉
PS: is everybody seeing a "blink" of the admin menu upon page load, or is it just me? ...although pre-existing issue; not related to this PR.
PPS: there is this strange behavior of the "Home" menu where it lingers for a second or two after hover-out. Also preexisting issue: #466
The "Home" link does not have a grey background on hover. Sub-links ave strange hover behavior too.
Fixed.
This Home link and the right ones (user, logout) could use some padding to have more space around.
All the links at the top should have the same padding. I added some padding to the whole bar so it's not quite so squished up against the edges of the browser.
I have left one comment in the PR re coding standards
Fixed.
One minor suggestion: why limit the adding of classes to only the top-level menu items?
Because that's the only place core is adding icons :)
If we added them to all, we'd be making it easier for people to add icons via their custom themes
Custom admin themes are pretty rare, and even then, a custom admin theme may prefer to add their own classes. This is not an 80% use-case.
plus would be one less conditional in the code 😉
Yes, but it would be a whole lot more muck in the markup! 😉
The "Home" link does not have a grey background on hover. Sub-links ave strange hover behavior too.
Fixed.
Indeed, but it so happens that the home menu does not get active-trail
, so doesn't get a gray background when in the home page. Although that is a preexisting issue, perhaps we should fix it here. Last detail and I'd call it RTBC 😄
the home menu does not get
active-trail
I had the same thought, but then decided against it. That class is only added to the menu items. Nothing on the right hand side gets it either. I think if we wanted to extend that functionality to different parts of the admin-bar, that should be a separate issue.
Sure, separate issue is fine: #3472
RTBC then 😉
All the links at the top should have the same padding. I added some padding to the whole bar so it's not quite so squished up against the edges of the browser.
some have a padding of 29px
, and some of 27px
; I think everything is much cleaner with a common 29px
padding.
some have a padding of 29px, and some of 27px ; I think everything is much cleaner with a common 29px padding.
Ah, I don't think that was intentional. After viewing them both, I liked the ones at 27px better, so I split the difference and made them all consistently 28px! :) PR updated.
Ah yes, golden line in the middle 😄 ...although I still see 27px and 29px in the PR. Did you perhaps forget to push @jenlampton ??
Yep. Pushed now :)
Still some mobile issue. Can we rename "more admin tasks" to "more tasks" ? ( we are already in the "admin" context). See screenshot:
Another little review https://github.com/backdrop/backdrop/pull/2456/files#r247350922
( gosh, this is hard, but I love these new admin icons ! <3 )
I incorporated @opi's last suggestions and merged in https://github.com/backdrop/backdrop/pull/2456. I left some notes there on how this could have been done differently, but I think this is suitable in any case as it's implemented now. Thanks @jenlampton, @klonos, @opi, and @stpaultim for putting this together!
I left some notes there on how this could have been done differently
...and I filed a follow-up so that this does not get lost in time 😉
Follow-up for #364 and part of #1839...
We should add icons to the Admin Bar. This will make Backdrop both more usable, and more beautiful.
Current Admin Bar:
Admin Bar with proposed icons:
I didn't like the Content icon (pencil) being right next to the Home icon (house). So I added the word "Home" next to the house icon.
I also only included them on the top-level links, and only on desktop (when we're sure there's enough room for them)
The one part of the PR that could use work is the addition of the classes. I'm currently adding a class to every link based on it's URL. It might be smarter to only add classes where they are needed (top-level only) and maybe name them something different? I liked these names because the paths will never change, where something like mlid could be different on every site.
PR by @jenlampton: https://github.com/backdrop/backdrop/pull/2456