Posted by Shyamala on November 24, 2012 at 1:44am
18 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | toolbar.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | frontend performance, Needs manual testing, Novice, Spark, toolbar-followup |
Issue Summary
Problem/Motivation
Adding an icon sprite for UAs that don't support data uris as arguments to the CSS url() function. This is part of toolbar followup further to code freeze.
Meta issue:#1846970: [meta] Responsive Toolbar follow-ups
Proposed resolution
To be determined.
Remaining tasks
To be determined.
User interface changes
None.
API changes
None.
Comments
#1
change status to active
#2
I don't know what UAs are, (User Agents?) but I'm guessing the point of this issue is to replace all the icon files in the toolbar module with a single sprite image? I think the active state versions could probably also be done with opacity rather than using a whole other icon.
Updating title. (If this is not what this issue is about, please comment so I can create a new issue) :)
#3
Yes you are right 'UA' is User Agent, got copied from the description. Thanks for the title edit!
Please go ahead and create another issue for
#4
It might be worth starting with a simpler approach, but Assetic should be able to handle sprite generation/formatting, once that is in.
#5
Here is a patch to get the ball rolling. This combines all the Toolbar icons into one sprite and changes the CSS to reference the sprite. I'm unsure about whether to include other icons in the sprite, like those provided by the Edit or Shortcut modules. Those icons were impacted by the change and haven't been repositioned yet.
#6
Adding tag novice.
This issue needs review.
#7
- Menu -> Help icon, some pixels cuts off
- the icons on sprite image placed too close. It should add more spacing between each icons. eg. when you increase the font size to 20px or add more paddings. the top & bottom icons will show on the menus.
- why all icons on the stylesheel missing ".toolbar" qualifier
- position of shortcuts & admins icons doesn't looks right.
#8
Tagging so we can keep track of this. Thanks for the patch, bryan!
#9
Tested patch on firefox 19.x and found below issues:
#10
Fixes for #5 patch:
- Background image repeat in css removed,
- Icon being cut for help and admin links changed
- Upper toolbar icons alignment fixed
- Some other icons position breaking fixed - orientation and dropdown icons
- Deleted unused icons
- Image sprite compressed with http://tinypng.org/
Please review. Tested on Chrome v24 on Ubuntu.
#11
+adding tags for testing
#12
Patch applies cleanly and tested with Firefox, chrome and Safari.
Repo of applied patch: https://github.com/ahimsauzi/Drupal-Ladder
#13
Awesome, thanks! Performance++
Assigning to Jesse to just give a once-over.
#14
Don't mean to block it move forward.
+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined
@@ -39,79 +42,89 @@
+ background-position: -1px -379px;
...
+ background-position: -1px -347px;
+++ b/core/modules/toolbar/css/toolbar.menu.cssundefined
@@ -87,15 +87,15 @@
+ background: url("../images/toolbar-sprite.png") 1px -503px;
...
+ background: url("../images/toolbar-sprite.png") 1px -474px;
+++ b/core/modules/toolbar/css/toolbar.theme.cssundefined
@@ -143,8 +143,8 @@
+ background: url('../images/toolbar-sprite.png') 1px -721px;
Just out of curious why these icons added "-1px". I'm only check the icon-home button. It's 21px but width of icon DIV is 20px.
Also about #7:
Also,
If icons meant to be used on everywhere, therefore this is remove ".toolbar" qualifier. Why not move it to CORE stylesheet ?
#15
@droplet,
I'm not sure about moving the icons to core stylesheet. And where exactly do you propose to move them?
Anyway here's a patch covering #14 issues.
I'd like it it be reviews asap, i'm working (actually done) on refactoring the css and js for toolbar module for the smacss principle and reducing the selector weight.
For example:
.toolbar .vertical to .toolbar-vertical
Hope this makes sens.
#16
#17
Can we implement the retina-sized version (200%) of the icons? I could make these if I can find the original full-size icons.
#18
Patched 8.x-dev. Wanted to see if it is faster. Still can be slow, but at least images aren't being painted 1 by 1.
#19
Created #1963886: Create a retina version of the toolbar for my comment in #17.
#20
@oresh,
Thanks, it looks better now.
Leave it to maintainers. :)
#21
Not pertinent but relevant. What is the source of the icons? Is there any notion of a 'drupal standard icon' collection? As more iconic menus show up in drupal core I would think there is an interest in a consistent ui for drupal core.
#22
What about creating an icon font?
We have played with the idea of creating an icon font for this sort of thing for Drupal and I am more than happy allocating time for it.
#23
@robin.prieschl, So, we talked about icon fonts.
Guys are trying to write an understandable document on using icon font. The thing is - everybody knows how to use sprites, but not a lot of people know how to use icon fonts. I've never done that before for example.
Also this issues is about sprites. Font icon will weight more then this sprite ( i guess ) and it needs specific documentation, when sprite do not. So if you want font icons search for this issue, if there's nothing about that, create a follow up :)
#24
@robin.prieschl: Yes, please open a new issue for icon fonts.
#25
I tried re-doing this with icon fonts instead of images or a sprite file. It is pretty straight forward but before I open another issue with a patch there are some things to think about. I don't see any performance or page size issues but using a font or a sprite file does mitigate the 'late' drawing of the images when the page is repainted that you see with individual images. The icon font solution does lessen the css required. The problems I wasn't able to or couldn't figure out how to address are:
1) The toolbar is actually built from 4 different menus from 4 different modules; toolbar, user, shortcut, and contextual. With the possibility of other modules, including contrib, chiming in. Currently each module does its own styling of added icons. So, any changes to toolbar icons have to be aware that other modules have to be considered, and any other module adding to the toolbar has to be aware it needs to do its own styling for icons.
2) There are accessibility issues that arise from using icons with links and they all apply to the current toolbar implementation. I don't see a straightforward way to address these in the current implementation. Are there any future plans for enhancing the toolbar module?
#26
I think we need to create an issue to address the icon link pattern.
Naming all icons in CORE and make the icons button/links html structure more consistently.
eg.
<i class="icon-flag"></i><a href="#">Link</a><i class="icon-menu"></i><button>Button</button>
It will be more easy to reuse/replace SAME icon everywhere.
#27
Review again, it's good enough now, other issue could work in a new issue thread. RTBC
#28
This does not convert the code in modules user, shortcut, and contextual to using the sprite file. You still get the late painting affect. Is it really better for those to be separate issues? You will still see the late painting affect.
#29
@dcrocks,
What is "late painting affect" ?
#30
In the current D8, there are times when the page you are on is completely redrawn on the screen. For example if you were in the admin overlay and made some change, saved, and after the overlay returned you click on the 'home' link. When the page is redrawn you 1st see the home page itself and the toolbar, but it only has the text. Then the icon images are painted on the toolbar overlay one by one with a perceptible pause between each. With the sprite patch you only see this on user and shortcut link icons because they are still using background images.
This is very evident on firefox(20.0), much less so on chrome(26..) and less on safari(5.1.9). I'm sure it is also a factor of my machine's age, old but not that old. I see it even when I use a fast server on a fast connection that is idle otherwise. Each image can mean a trip to the server, that's why sprites are better. I am beginning to think that it is firefox specific, but a lot of people use firefox.
#31
Ahh. missing 2 icons in sprite image.