Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It's easy to target devices with 1.5x or 2x resolutions. We should display retina-like resolution for the toolbar images.
Anyone can tell me where to find the original files of the 1x icons (ideally the vector version)?
Once #1849078: Replace many toolbar icon files with a single CSS sprite image done. We could create a new sprite the 2x version of the icons and do the remaining CSS.
[update]
We are exploring using SVG icons or an icon font to display the toolbar icons.
Comment | File | Size | Author |
---|---|---|---|
#111 | Screen Shot 2013-08-28 at 1.43.45 AM.png | 134.76 KB | webchick |
#111 | Screen Shot 2013-08-28 at 1.47.04 AM.png | 61.92 KB | webchick |
#106 | interdiff.txt | 119.96 KB | jessebeach |
#106 | toolbar-hdpi-icons-1963886-106.patch | 104.2 KB | jessebeach |
#98 | toolbar-hdpi-icons-1963886-98.patch | 113.97 KB | Wim Leers |
Comments
Comment #1
oresh CreditAttribution: oresh commentedI think you can find the icons in the modules/toolbar/images before applying patch from #1849078.
Have no idea about vector, may be somewhere. I haven't seen them.
There's a good article explaining how to make the image look good for retina and yet keep it small. I'm not sure if it's applied to png, but anyway: http://blog.netvlies.nl/design-interactie/retina-revolutie-follow-up/
If you provide 1.5 and 2.0 scaled icons for retina displays - I can add them for the toolbar.
Comment #2
xmacinfoI am looking for the source files to generate the icons in their larger versions. Without a good vector based source file, I would need to recreate all images.
For pictures like shown in http://blog.netvlies.nl/design-interactie/retina-revolutie-follow-up, using a single file is the way to go. But that does not apply to icons; the scaling would blur some details.
Comment #3
oresh CreditAttribution: oresh commentedWe were thinking of creating icon font for that.
Contact me this evening (PST time) / or find me on irc #drupal-mobile - i can help remaking the icons in vector, so you can use it for retina and in future for the icon font if needed.
Comment #4
xmacinfoAn icon font is definitely better. But then, again, we would need the source vector file.
Do you know the issue number for the icon font toolbar icons?
Comment #5
oresh CreditAttribution: oresh commentedI think issue for icon font has not been created yet.
I don't the source files, I can just redraw them in vector this evening :)
I would like to include some additional icons to the font, which are not used yet, but can be in future.
Comment #6
jessebeach CreditAttribution: jessebeach commentedWe also have SVG versions of all the icons if you want to go that route instead. We can add a Modernizer SVG check into the Toolbar JS and use that to determine support. Could be a middle road solution if creating a font seems daunting.
Comment #7
xmacinfo@jessebeach: can you give me and oresh access to the SVG files?
So instead of creating the retina-sized sprite, we could either use the Modernizer SVG route or the icon font route.
Icon fonts, once the file is created, are easy to use. The bad side to this, though, is that a character needs to be used for an icon to display. Looking at the source may be funny.
I guess I need to change the title of this issue.
Comment #8
dcrocks CreditAttribution: dcrocks commentedI've created a patch to convert toolbar to use an icon font instead of images. I've attached some images as well but there are some comments necessary. I used Icomoon to create the font in the patch, so they don't match the original images. I just needed something to work with. There are some issues in 'active' state logic that I didn't address. If you flip thru the menus you will see they don't behave as expected. I didn't use an embedded empty <i> element. I have figured out how to apply them to menu items but not to buttons yet. The 3 reasons I'm waiting are
(1) What is desired design for rtl placement of the icons: left of text for ltl and right of text for rtl?. If so I need logic to place the <i> element appropriately.
(2) Following the lead of #1849078: Replace many toolbar icon files with a single CSS sprite image I haven't redone the shortcut, user, and contextual modules to use icons in their toolbar support.
(3) Using <i> will require substantial reworking of the css, since it needs to be applied to a different element.
I haven't checked this on any mobile devices or very many browsers. There is a small problem with chrome that I didn't take time to look at. So please try it. This does make the issue retina screen devices moot.
Comment #9
dcrocks CreditAttribution: dcrocks commentedI didn't get the font files into #8. Another try
Comment #11
xmacinfoWhat do you mean by moot?
I have not tested this yet. But it looks promising!
Comment #12
xmacinfoI would rename icon font files to :
icon_drupal.woff
icon_drupal.ttf
etc.
Comment #13
dcrocks CreditAttribution: dcrocks commented#11 Since we are dealing with text, image dimensions are not a concern. But icon quality still is.
#12 Good thought.
I need to figure out what I did wrong adding those files. The patch is worthless without them. At least I know the rest is OK.
Comment #14
xmacinfo@dcrocks: Have a chat with maintainers to check if those file format are allowed. I do not know.
Comment #15
dcrocks CreditAttribution: dcrocks commentedI think I need to tell git that all the font files should be drupal binary. Can I do that locally or do I have to do that globally somehow.
Comment #16
dcrocks CreditAttribution: dcrocks commentedAnother try. No changes, just trying to get files loaded.
Comment #17
xmacinfoIt passed the tests.
File name change would be nice for the font files.
Comment #18
dcrocks CreditAttribution: dcrocks commentedI'll do that in next patch. I need to test it further. There aren't really any tests to show the icons are working as expected. Does anyone have an icon font to propose here? Or svg files for the previous set?
Comment #19
dcrocks CreditAttribution: dcrocks commentedThe patch seems to be good but I would appreciate it being tested on other devices, I've attached an image of the content of the font set I'll use in the next patch, if desired(my english teacher insists I call it drupal_icon). Any suggested additions. This is still a very small font. Any other comment would be appreciated.
Comment #20
Hanno CreditAttribution: Hanno commentedGood work to replace the images with svg fonts.
I am wondering if this affects screen readers. The CSS-element 'speak' is used that it isn't read aloud, but there is only one browser that supports this: http://csscreator.com/content/speak
Is text to speech tested?
EDIT: I see the Private Use Unicode subset is used for the characters, so it shouldn't affect screen readers.
Comment #21
xmacinfo@dcrocks: Yes, we need to test on other devices, not only retina ones. Your English teacher is right for the files names.
@Hanno: I do not expect any adverse effects on screen readers. But as you pointed, we will need those to be tested as well.
I do not have time to do testing yet, though.
Comment #22
jessebeach CreditAttribution: jessebeach commentedSorry to have been so silent. I'm assembling the SVGs now into a zip.
Comment #23
dcrocks CreditAttribution: dcrocks commentedI have been getting the icons from Here . If I can get the svg files I may be able to build a font from them plus other icomoon icons. Then these efforts could work from a common set.
Comment #24
tkoleary CreditAttribution: tkoleary commentedHere is an *almost* complete set. It covers all of the current toolbar and then some, plus a new puzzle piece icon for extend per this issue: http://drupal.org/node/1891096.
The ones in the wysiwyg folder are icons we made when we were still leaning toward Aloha, I am thinking I should revisit this work to cover all icons in CK and contribute back to them, (or maybe we just build a custom Drupal skin for CK with retina icons ?)
Comment #25
dcrocks CreditAttribution: dcrocks commentedI can use the app at icomoon to build an icon font for the toolbar from the files in #24. It will take a little time, then I can try to preview it here. One thing to note is that for most of the toolbar icons a state change is simply indicated by a change of color. When using a font, the same character with "color: #xxx;" can be used to indicate that change. In fact, it actually works out quite nicely with the existing css.
Comment #26
dcrocks CreditAttribution: dcrocks commentedHere is a look at the generated font. I only used the 'normal' characters and didn't add the 'wysiwyg set for now.
Comment #27
dcrocks CreditAttribution: dcrocks commentedClearly a problem with the 'close' character I didn't notice before. This character isn't in the current implementation. What is the intended use?
Comment #28
tkoleary CreditAttribution: tkoleary commented@dcrocks
Al steffen, Preston So and I explored the same approach you are taking with icomoon right now but we foresaw a problem with icomoon that prevented extending it to the community which I don't *think* icomoon has solved.
The issue is that when we create a font for icons in core we want to be able to:
Icomoon AFAIK will not work for this because the user does not have control over which PUA cell the icons are placed in (or at least not consistent and granular control).
An open-source application like font forge (http://fontforge.org/) will permit this but we will also need to institute some community rules, tools and best-practices and around this.
They might look something like this:
Rules:
Tools
Best Practices
Not totally sure but basically, document well and don't be a jerk. :)
Comment #29
tkoleary CreditAttribution: tkoleary commentedThis would be the global "close" icon across all modals and overlay. It can also be used for things like clearing fields, removing tags, etc.
See the new seven style guide and dev sandbox for more info.
http://drupal.org/project/issues/1932040
http://groups.drupal.org/node/283223
Comment #30
tkoleary CreditAttribution: tkoleary commented@dcrocks
And specifically this issue: http://drupal.org/node/1938410
Comment #31
xmacinfoAgreed. With an icon font, CSS can be used to changed the color.
But how can we create the gradient used for the toolbar? CSS can do gradients, but it may be hard to add gradient to the fonts and still be compatible with old browsers.
Comment #32
ry5n CreditAttribution: ry5n commentedThere has been some effort to come up with a markup/css pattern for implementing icons in a consistent way: http://drupal.org/node/1849712.
Edit: BTW, if anyone thinks the issue mentioned above makes sense for D8, please chime in. @Mark Carver has jumped into that issue and insists it should be postponed to D9.
Comment #33
xmacinfoMoving back to regular PNG for the icons, though a retina-based set.
The issue http://drupal.org/node/1849712 already deals with SVG and font file. All discussions about font file or SVG should move over there.
Creating a twice the resolution image sprite should still be done to have a retina-ready toolbar, along with the required CSS. We can do this now that we have the vector icons.
Comment #34
dcrocks CreditAttribution: dcrocks commentedRe: #28. I don't think it is desirable to try to 'reserve' cells in the PUA. The only thing that matters is which font is affect when the browser tries to render a given character on the page. The important thing for any implementer or user is that the glyph he expects is called for the token he used to select it. Even if you are planning to chain icon fonts, you can insure your glyph is chosen by making your font a higher priority in the font-family list. Adding icons to drupal as a generalized solution is going to be very difficult if not impossible. If there is one place in drupal that all candidates for icons go through then it may be too expensive to implement the added code. Icons are implemented now in drupal in different places in different ways. Generalization would be nice, but difficult.
Again, I don't think reserving cells in the PUA is desirable or even necessary. Having such a large community as drupal trying to 'reserve' positions in PUA doesn't seem workable to me. Using a common tool would also be nice but that is going to difficult as well. For example, the version of Font Forge for the Mac hasn't been updates since 2009. For sake of a consistent UI, drupal Core modules should all work off the same icon font. But after that I think it should be wide open.
Comment #35
dcrocks CreditAttribution: dcrocks commentedSorry, didn't mean to change the status.
Comment #36
xmacinfoCross post.
Comment #37
xmacinfoAgain.
Comment #38
tkoleary CreditAttribution: tkoleary commentedWe should also add the glyphs from the Drupal wordmark. See issue #605710
Comment #39
Bojhan CreditAttribution: Bojhan commentedIt seems like this is impacting the style guide, adding tags.
What's the hold up here?
Comment #40
tkoleary CreditAttribution: tkoleary commentedRe: "I don't think reserving cells in the PUA is desirable or even necessary."
Can you clarify what would happen given the following use cases?:
"As a sitebuilder I want to install module "foo" in my site. Module "foo" includes an icon font and CSS to display an icon in it's administrative page. Sometime later I install module "bar" in the same site. Module "Bar" adds a field to the UI of module "foo" that includes an icon (a different icon than the one being used by module "foo") The two icon fonts use the same font name and take up the same PUA cell."
"As a sitebuilder I want to build a site that uses a large number of modules. 20 of them have their own icon fonts. How can I make this performant?"
"As a module developer I want to add an icon font to my site. I do not know what icons are already available or in use by other modules. How do I find that out?"
Comment #41
dcrocks CreditAttribution: dcrocks commented#1: Even though a font-face family may have the same name as another I would not expect each to have the same source. I would expect the current '@font-face' definition in affect in the cascade would define which icon was produced. Module 'bar' needs to make sure its font-face is in affect when its field is output. This is just a css question about fonts, not just 'icon' fonts.
#2: Substitute just 'font' for 'icon font'. Is the question any different? Is it any different if they all had different sprite files? Or if they all had their own image sets?
#3: How do you find out what other modules mapping into the 'PUA' is? How do you enforce a rule that says every contributor to drupal must 'register' their mapping of the 'PUA" against a 'Drupal' one? How big is the 'PUA' compared to the number of drupal sites across the entire world? Do we force contributors to make sure their font-face names are unique against some 'Drupal' collection of names? Or that their image/sprite/file/anything names do not collide with some other contributors choice?
I don't think it is at all realistic to enforce a 'PUA' mapping across all of drupal. It may be realistic, even desirable, within a given project.
Comment #42
xmacinfo@tkoleary and @dcrocks: The issue http://drupal.org/node/1849712 already deals with SVG and font file. All discussions about font file, font icons or SVG should move over there.
The goal of this issue is simply to create the retina-version of the image sprite.
Comment #43
mgiffordWould be great if there was a git repository with the SVG files for D8. It should probably be it's own project in Drupal.org. I believe that all SVG files can be tracked via version control too so it would be a great resource for everyone. I'd actually like to see SVG files for all of the design work in Core (where possible) so that we can more easily customize it going forward. That would be better than uploading a zip file, but a bit more time consuming.
Comment #44
ry5n CreditAttribution: ry5n commented@xmacinfo #1849712: Add theming template and prepare logic for rendering icons was about providing standard (overridable) theming in core for icons of all types. It doesn’t deal with actual icons and isn’t specific to SVG or fonts. It also likely will need to be moved to 9.x now that code freeze has hit.
That said, if an icon font is still under consideration for the toolbar, I built one with coverage for all of core’s current icons, including the toolbar. See #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core.
Comment #45
tkoleary CreditAttribution: tkoleary commented@mgifford I think it's a great idea to have the files freely available in a repo, I don't think the actual files need to be in core though, since we can just base64 encode them and put them right in the css. Arguably all icons in core should be in a single icons.css file.
Obviously this will not be necessary if ry5n's iconfont gets in.
Comment #46
xmacinfo@ry5n: Any icon fonts/vertor icons/svg icons would need to be moved to 9.x.
However, creating a retina-sized version of the PNG of all the toolbar icons is not affected by the code freeze. We just need to recreate the sprite/icon file and update the CSS accordingly. That type of correction is not affected by the freeze.
If #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core makes it for Drupal 8, we will not need to complete this issue. But that's an “if”.
Comment #47
tkoleary CreditAttribution: tkoleary commented@xmacinfo
My understanding is that UX polish of this kind is precisely what we should be working on now and is not bound by API freeze. In my understanding implementation of this consists of CSS changes and an added file (the font). Could you clarify the API iconfonts/svg icons would touch that makes it subject to freeze?
Comment #48
Gábor HojtsyToolbar is a part of Drupal core that we explicitly want contributed modules to extend. Changing how things can be extended (what kinds of images would be needed in what style, etc) is what defines an API change. I think its clear we need to solve this because the toolbar look bad on retina displays (so I've been told, I don't think I own one), however it is important to do it with least changes possible as well as soon as possible to let contrib catch up instead of need to rewrite stuff.
Comment #49
Dragan Eror CreditAttribution: Dragan Eror commented@Gábor Hojtsy
Retina displays are in all recent cell phones and tables (not just in apple products) I believe that we all own at least one ;)
According retina icons: I agree that this task should be fixed and retina ready toolbar released with Drupal 8, but also we should consider all other icons which comes with core. Making just toolbar retina ready doesn't make sense.
Comment #50
Gábor HojtsyAll right, I checked on my recent android phone as well, the icons do look pretty bad...
Comment #51
tkoleary CreditAttribution: tkoleary commented@Dragan Eror
That's correct. This should be extended to all icons in core, however there are relatively few of them so I don't think its a big task.
Comment #52
jessebeach CreditAttribution: jessebeach commentedThe shortest throw solution here will be to take the icon SVGs that ry5n has created and introduce them to the CSS files as base-64 encoded background images. That way, we'll have the scalability of SVGs in an overridable format.
Comment #53
tkoleary CreditAttribution: tkoleary commented@Dragan Eror
Looks like there are about 30. I am making a comprehensive list now. Many are dupes of the ones in toolbar.
Comment #54
tkoleary CreditAttribution: tkoleary commented@Dragan Eror
I have gone through them all and the only icons in core—not including theme specific icons—that are not represented in Libricons are:
There are also the 14 file type icons in files. Those could be added as well.
Comment #55
Wim LeersIt's not as simple as "let's create an icon font that covers our current icons and consider it done".
Requirements
Approaches
The current approach and an alternative approach. If you can come up with another, please comment.
Approach 1: "just use an icon font"
This is the current approach.
, and with a
@font-face
declaration for.iconfont
so that the inserted character uses the icon font.Approach 2: "modules define their (SVG) icons; modules and themes can override them; build CSS dynamically"
.svg
file per icon). We could then automatically generate a single CSS file with all icons embedded as data URIs (like #52 already says, but using URI encoding).However, a side-effect of this approach is that we need some sort of hook or
.yml
to indicate which icons this module provides. How else can Drupal know how to generate that CSS file? It's less than ideal that we need another "metadata thing", this time for icons, but at the same time that is also utterly essential.How else will modules be able to override existing entries? How else can a theme override only Drupal core's default icons as well as the icons for modules a and b, and still have the system serve up the default icons for modules x and y?
A contrib module could then *still* override the default CSS generator and generate an icon font instead. Or a site-specific theme could override the default icon CSS file and embed its own optimized icon font.
Comment #56
jessebeach CreditAttribution: jessebeach commentedThis feels like pre-optimization. If we simply replace the existing CSS background image url() functions that reference files with an encoded image reference instead, then we have introduced scalable images with the same level of overridabability that we have today. We don't need to tie the effort to introduce scalable font images with the effort to build a system that makes them swappable.
Comment #57
Wim Leers#56: great point, but we need to plan ahead. Part of the appeal of icon fonts is that it's a single HTTP request. So we need to be able to do the same with whatever approach we take, even if we don't do it in the first round.
Comment #58
Gábor HojtsyMy understanding is that solving this in different stages will change the toolbar extendability API multiple times, no? The question is with each of those changes, would a toolbar extension module written today still work and then what kind of changes would be needed.
Comment #59
yoroy CreditAttribution: yoroy commentedI think the icons look pretty crappy on retina and non-retina screens alike :-)
To me they add value at the top level: Home, Menu, Shortcuts, User.
But the ones for Content, Structure, Appearance etc. are mostly visual noise and they don't really help discerning things: Appearance, Extend, Configuration, People and to a lesser degree Reports all have a similar composition: a top-left to bottom-right diagonal.
Icons are mostly used as visual hooks, little spots that attract attention, we can't rely on them to communicate meaning. In their current state I don't think these second level icons live up to the screen real estate they are taking up.
Point being: yes, make things look good on hi-res screens, but focus on where it makes sense to add icons in the first place. The best scaling strategy is to not have icons for the second level Menu items. Which may be a different issue…
Comment #60
xmacinfo@Wim Leers: The approach 1 you described is very close to the #1849712: Add theming template and prepare logic for rendering icons issue, and thus, we would need to mark this issue a duplicate and move all discussions over there. I still believe that discussing icon fonts would be Drupal 9 stuff.
As for Approach 2, or doing a simple high-res sprite image as the original intent of this issue, we are waiting for #1938044: Prefix all toolbar classes to prevent theme clashes to be done.
@yoroy: I agree 100% with you. We should remove all icons on the second level menu. But that would warrant another issue.
By the way, this issue would love an updated issue summary. :-)
http://drupal.org/issue-summaries
Comment #61
Wim Leers#60: the approach #1 I describe is the one that was being discussed here already and I believe it to be flawed. Please read my comment again: approach #1 fails to meet all requirements!
Comment #62
xmacinfoI have read that more than once. I just pointed out that for that approach, requirements or not, is discussed at length in #1849712: Add theming template and prepare logic for rendering icons.
Comment #63
Wim Leers#62: oh :) Sorry! I thought you meant approach #1 was the right one to take. Which it is not, IMO. Apologies!
Comment #64
Wim LeersAny feedback to #1963886-55: Use HiDPI icons in the toolbar? Particularly approach 2, which is the solution I propose.
We need to start moving forward on this.
Comment #65
tkoleary CreditAttribution: tkoleary commented+1 to
While I would love to see an icon font in core, this approach appears more flexible and achievable ATM and does not complicate iconfonts in D8 contrib admin themes.
Comment #66
webchickTagging.
Comment #67
jessebeach CreditAttribution: jessebeach commentedWhen we first got the Toolbar committed, we didn't yet have Modernizr in core. We didn't have a way to use SVG icons without leaving Android 2.3 devices without any icons. This article is a bit long, but it's comprehensive and it explains the impasse we were up against 8 months ago, but that doesn't impede us any more: http://css-tricks.com/using-svg/
So, now that we have Modernizr, I've put in SVG versions of the icons as the default. We use the Modernizr
no-svg
class to create a fallback for PNG icons for IE8- and Android 2.3. I had to introduce an SVG feature support test to our custom Modernizr build.This patch is still
needs work
because I'm missing SVGs for contextual module and a few of the smaller icons (disclosure toggles) in Toolbar. Kevin should be back early next week to get those icons to me.Comment #68
jessebeach CreditAttribution: jessebeach commentedAdding
toolbar-followup
tag.Comment #69
Bojhan CreditAttribution: Bojhan commentedFeel free to use https://drupal.org/node/2032773, Kevin, me and lewis already agreed to go with this icon set.
Comment #70
jessebeach CreditAttribution: jessebeach commented@Bojhan, the icon-font approach still has a couple major unanswered questions.
Have you all addressed these questions already? The SVG approach here is ready (almost, minus some missing icons SVG files). Is the icon font patch ready to go? Or close? If so, I can direct my attention to it instead.
Comment #71
Bojhan CreditAttribution: Bojhan commented@jessebeach Actually I wasn't arguing for a particular method - I am happy to go with whatever you choose. I was merely trying to provide graphics you were missing. There are sources files in that issue.
Comment #72
jessebeach CreditAttribution: jessebeach commentedAh! I misunderstood! Thanks for the pointer. I'll use those icons.
Comment #73
tkoleary CreditAttribution: tkoleary commented@jessebeach
Yes, we discussed using Ry5n's libricons in UX happy hour. Sorry, I should have updated you on that.
FYI in Ember theme and Spark distro we will be overriding those and using Acquicons, the set that Drew and I are working on. This should also be a good case study for implementing a global icon override for any Distro developer or admin themer in D8. Maybe we should do a webinar?
Comment #74
Wim Leers#73: a webinar may be useful in six months or so, but right now nobody is building production stuff on D8 yet, so it won't be useful yet. Once D8 is out, it would be a very valuable webinar though :)
Comment #75
jessebeach CreditAttribution: jessebeach commentedAlright, this patch introduces SVG icons, taken from the libricon set. I updated toolbar, shortcut, user, contextual, and tour. Every module that has a toolbar tab.
I must admit, I've love to has a solution that let's us vary the coloring of an SVG through CSS, but I just don't see how to do that consistently across devices right now given the current feature support overlaps and gaps. So I went with a brute-force solution of providing an SVG file for a specific color when needed. I didn't fill out the full set of possible icons we might need: just the ones we're actually using. I didn't want to pre-optimize.
The icon sizing might need some adjusting.
RTL and .no-svg are provided for in all cases.
Comment #76
Wim LeersHaving tested this patch on simplytest.me, it seems that the icons are too small? Especially the pencil icon in the top right corner.
Also, the pencil icon on contextual links is still using the PNG file — that should be using the SVG too, and the PNG should be deleted.
What's the meaning of this particular directory structure and file name structure?
What's the meaning of the
toolbar-contrast
class? Light-on-dark icons?Each of these icon CSS files be defined as libraries, so that dependencies can be declared.
Comment #77
tkoleary CreditAttribution: tkoleary commented@WimLeers @LewisNyman
Lewis, does it look right to you?
Comment #78
tkoleary CreditAttribution: tkoleary commentedTagging with Seven theme
Comment #79
jessebeach CreditAttribution: jessebeach commentedI was hesitant to touch the contextual module icons (because where do you stop pulling that particular thread for replacing icons?), but you make a good point that I'm introducing two versions of the same icon -- the edit pencil. I'll replace them across the board.
The hex number in the directory path is the fill color for the icon. The icon names are pulled from ry5n's repo. I assume the prefixed digits refer to Unicode slot IDs? IDK, but I didn't change the file names.
Right the help icon is used in both light and dark versions and I needed a way to differentiate. I wanted to be consistent with the other icons in the toolbar bar then.
Right, I left that code as is, trying not to change too much. I had to hold my nose a little. Probably I was the one who wrote it originally as well :)
Updates and slightly larger icons coming soon...
Comment #80
jessebeach CreditAttribution: jessebeach commentedI've replaced the contextual link icons with the new pencil icon. I wasn't able to delete edit.png because we need it for the .no-svg fallback.
All references to icon css files used in this patch are now included through libraries instead of just included the CSS files directly on the render array.
I've upped the size of the icons to 100% of their container, which was sized to fit the PNG icons. So the SVG icons now have roughly the same size as their PNG counterparts.
Comment #81
Wim LeersMuch better! See:
However, to my eyes, the pencil icon is now much less recognizable as a pencil… or is that just me?
Secondly, when you resize to narrower widths, the SVGs now blow up to massive (yet very crisp! :D) proportions:
I think we might want to have *separate* libraries: a
drupal.shortcut.toolbar
library (with just the theme CSS file) which then depends on adrupal.shortcut.toolbar.icons
library (with just the icons CSS file).This makes it easier to override just the icons. Or doesn't it?
Comment #82
jessebeach CreditAttribution: jessebeach commentedI've fixed the ginormous icons in the "tablet" screen size by applying a
background-size
to them. Any user agent that supports SVGs is going to supportbackground-size
as well.Wim, I addressed the Shortcut library comment in #81 by declaring a library for the theme CSS and a library for the icon CSS and including them both in the renderable returned by Shortcut in
hook_toolbar
.Comment #83
LewisNyman CreditAttribution: LewisNyman commentedI tested on a range of devices, I forgot to take my Android 2.2 with me this morning though. If you're looking for a specific device test range best chuck this up in the issue summary.
In general the icons look very good! For some reason the home icon looks a bit blurry on FIrefox, it is loading SVG though.
One other general thing I noticed is there is occasionally a delay when loading active state icons over a network. The area can appear blank for a small time and this could be worse on slower connections. It's worth thinking about because at best it makes the UI feel slow to respond and at worst it might confuse the user.
You might of discussed this already but maybe it makes sense to fall forward to SVG instead of backwards to PNG? I think it's likely that any device we don't support for JS would not support SVG.
It looks like we are using completely different icons for the png fallback. I don't think this is our intent?
It would make sense to me to have the png equivalents sitting alongside the svgs. So the only part of the path than changes is the file extension.
I'm not sure I understand the use of the 'toolbar-contrast' class.
Comment #84
Wim LeersLewisNyman: blurry SVG in Firefox is a bug in Firefox, fixed in version 24, to be released soon: http://stackoverflow.com/a/11297217, https://bugzilla.mozilla.org/show_bug.cgi?id=600207.
Comment #85
jessebeach CreditAttribution: jessebeach commentedThat's a very interesting point. My thinking here is that we want to support the emerging standard (SVG) as the default, but because we're making the fallback distinction through JS, we are setting up a situation with a UA that won't have JS and won't have icons.
My thought on that situation is that if JS is not available, much of the admin UI won't work. And since these icons concern only authenticated users, I'm going to assume that they will have JS available and enabled. Personally I'd rather support the emerging standard as the default.
It just occurred to me that if we qualify the SVG selectors with a class added by JavaScript, we might run into a situation where UAs start downloading the PNG icons during a pre-optimization step before the
.svg
class is applied to the html element and the switch over to SVGs is triggered.Given those reasons, I'd like to stick with the code as it stands now.
I'm using @ry5n's SVG icons from the Libricon library: https://github.com/ry5n/libricons
They don't have PNG equivalents, so I just left the original PNGs in place. Realistically, only folks using IE8 to admin their site or compose content are going to see the PNG icons. I agree, though, that it does create a weird experience if you see Drupal from IE8 and then from a modern browser. It might feel like a different application.
Do we have a way of quickly batching the existing SVG icons into PNGs? I'd be totally impressed and indebted to anyone who could script that :)
I struggled with how to make this distinction. So, the help icon is used against both a dark and light background. In both cases, the element specifies
.toolbar-icon-help
. I needed a way to distinguish the light-colored icon from the dark-colored icon, so I added a.toolbar-contrast
class to all the tab elements in the bar (dark background) in order to differentiate them as selecting a light colored icon, rather than have different selectors for the help icon e.g..toolbar-icon-help
and.toolbar-icon-help-contrast
. I'm open to changing this approach.Comment #86
LewisNyman CreditAttribution: LewisNyman commentedI was thinking that to, let's leave it be then.
Prepare to be impressed! To batch convert SVGs to PNGs, you can use Grunticon or it's webapp equivalent: Grumpicon. I don't see it as a user issue, it just seems a bit unusual to maintain two icon sets.
Maybe a class that communicates the background is dark would be more readable? If I saw a selector like
.toolbar-container-dark .icon-something
, I think I'd immediately know why that declaration exists. The contrast class kind of threw me off because I couldn't think of a situation where you would want more contrast, if not an active state.I also just noticed that shortcut.icons.css is not loading in the overlay, which looks like this:
Comment #87
jessebeach CreditAttribution: jessebeach commentedAll of the SVG icons have PNG equivalents now. The PNGs have all been smushed. I deleted all the images in the Toolbar images directory, as well. All the icons are being pulled from
misc
.I just scoped the contrasty icons to
.toolbar-bar
. It's simple and it works.I missed the inclusion of
shortcut.icons.css
with a particular#attached
key on a render array. It's fixed now.Alright! That should be all the issues addressed. A green test run, a quick sanity check and we should be good to commit!
Comment #88
Bojhan CreditAttribution: Bojhan commentedI am afraid to do a RTBC on this. But this looks incredible! The icons look much more polished and sharp.
We gotta fix the star, that should be filled. Not part of this issue though, but maybe a follow up could be made?
Comment #89
jessebeach CreditAttribution: jessebeach commentedLo, the solid star! ry5n had an icon for this one, too.
Comment #90
LewisNyman CreditAttribution: LewisNyman commentedJust about to mark this RTBC but I think we need to include the Libricons license? https://github.com/ry5n/libricons/blob/master/license.md
Comment #91
Bojhan CreditAttribution: Bojhan commented@Lewis I am not sure, but I dont think so - we are redistributing it as GPL.
Comment #92
aspilicious CreditAttribution: aspilicious commentedSmall issue probably not cause by this patch. When you click on the icon to switch the horizontal and vertical mode (AND DON'T MOVE YOUR MOUSE WHILE CLICKING :p) you'll see that the icon stays "active". When you move your mouse it gets deactivated again. Not sure if it's introduced here.
Anyway, nice work!
Comment #93
webchickMIT can be re-licensed as GPL, since MIT is basically a "do whatever you want" license, including "limit the rights of this code further."
I think Lewis is asking for us to check in the actual license file along with the icons. This would be consistent with what we do for stuff under core/vendor (e.g. Symfony, PHPUnit). For front-end code (Modernizr, Underscore, etc.) which are also distributed under MIT, we seem not to do that — the license is embedded in the source code. Not sure you can do that with SVG icons.
If the intent is for these icons to be used by modules other than Toolbar (which I think is true?), then maybe moving them up into core/misc/icons or core/vendor/libricons is the best approach, and include the LICENSE.md file there.
We should also update http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/COPYR... with copyright info.
Comment #94
jessebeach CreditAttribution: jessebeach commentedThis patch in #89 has the icons located in
core/misc/icons
and I've removed all the images from the Toolbar module. So happily they are already in a common location.I added the license from the libricons repo to the
core/misc/icons
directory. I'm not sure if we should move these icons into theassets/vendor
directory because we haven't cloned the repo; we've created derivatives of the source icons.Comment #95
jessebeach CreditAttribution: jessebeach commentedGo bot, go.
Comment #96
LewisNyman CreditAttribution: LewisNyman commentedIf we are creating a variant of Libricons maybe a fork maintained on Github would be appropriate? That would mean we could treat it exactly the same as any other third party library.
Comment #97
jessebeach CreditAttribution: jessebeach commented@LewisNyman, we haven't really created a fork of the repo. We've taken source art from a project that licenses us to do so, and expanded that art into new variants -- those needed to satisfy our current requirements.
I want us to head in the direction of adopting the iconset wholesale, as a complete third-party project, once we have better integration for configuring the icons and styling them. We're stuck in an awkward place now, though. We want the look of the icons, but we can't have the instrumentation just yet. So we take the art and not the code.
ry5n can you chime in on this issue?
Comment #98
Wim LeersReview
Changes
I reverted this: it's worse after all. You then end up with a library for each CSS file, which is most definitely bad. For overriding all icons, and only all icons, we still have
hook_css_alter()
:)Comment #99
jessebeach CreditAttribution: jessebeach commentedMy process was nothing more than opening the SVG files in a text editor and editing the
fill
attribute hex value.My hope is that this is a one-time, stop-gap measure to get scalable icons into Core. I didn't want to invest too much time into a process here. And because these are text files and not binaries, they're at least easy to alter.
Comment #100
ry5n CreditAttribution: ry5n commentedI’m happy for Drupal to use these in pretty much any form:
1. Using only the svg art. Just put the license file along with the svgs in whatever directory. (BTW, the naming of files starting at 9999 in the Libricons repo is a super-terrible hack to ensure that re-importing the icons into Icomoon for modification will import them in the correct order. So please, by all means remove these numbers from the file name!)
2. Using the icon font, but with different CSS to implement it. Even though I argued for it, I’m not convinced the class naming pattern in the current repo is the best approach. Again, just include the license with the font files.
3. Using the project wholesale, icon font, CSS (obviously, include the license file).
I made this for Drupal, so if there’s anything else. just ask :)
Comment #101
jessebeach CreditAttribution: jessebeach commentedry5n, I really appreciate your work putting these images together. They're quite beautiful.
Is the inclusion of the license in the most recent patch satisfactory?
Comment #102
ry5n CreditAttribution: ry5n commentedYup, looks good. At a later date I may ask to add a readme that points to the Libricons repo. I assume there would be no objections to that. In any case (just reiterating) yes, you’re good to go.
BTW if there are individual icons you think need some tweaking (pencil icon e.g. may not be recognizable enough), let me know.
Comment #103
Bojhan CreditAttribution: Bojhan commentedWhoo :) Lets do this , happy that we now included it correctly - I wasn't sure.
Comment #104
Wim LeersMy only remaining question in #98 was answered in ##9, so no more answers here either :)
Great job Jesse!
RTBC +1
Comment #105
Wim LeersActually, this should be fixed first:
Comment #106
jessebeach CreditAttribution: jessebeach commentedI've removed all of the 9### number prefixes from the image files and updated their references.
Comment #107
LewisNyman CreditAttribution: LewisNyman commentedLooks like there is one place we are still using the original images, is that intentional?
Comment #108
jessebeach CreditAttribution: jessebeach commentedThe purpose of this issue is to make the icons in the Toolbar module scalable. I'd like to avoid replacing all the icons in all modules if we can. That scares me :) So the line has to be drawn somewhere.
Comment #109
LewisNyman CreditAttribution: LewisNyman commentedOf course! Let's do this.
Committers: please remember to include credit to Ry5n as he created the icons. Thanks!
Comment #110
Wim Leers#106: You may want to use
git diff --find-renames
next time, the interdiff would have been only a fraction then :)Removing the "needs accessibility review" tag: this makes no accessibility-related changes: an SVG is in fact just a different type of image. Accessibility review would only have been necessary if we'd gone for icon fonts.
RTBC +1 :)
Comment #111
webchickDude! These icons are HOT: (old icons top, new icons bottom)
Get a little cloooooserrr....
Wicked!
Committed (not pushed, because pushes are apparently disabled atm—will do so tomorrow) to 8.x. Thanks!
PS: I credited ry5n first. :) Awesome work!!!
Comment #112
LewisNyman CreditAttribution: LewisNyman commentedWoo! Now we can add Libricons to other elements in core. I've updated #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core and I'll be creating follow ups soon.
Comment #113
Wim Leers.
Comment #114
Wim Leersd.o tag monster
Comment #115
star-szrJust wanted to say great work on this ry5n, jessebeach, dcrocks, Wim Leers et al - looks amazing! :)
Comment #116
dcrocks CreditAttribution: dcrocks commentedSince I wasn't involved in the SVG solution, I don't think I deserve any credit for this. But I hope I can contribute to the use of icon fonts in drupal in the future.
Comment #117
nod_This adds a modernizr test, tag.
Comment #118
tkoleary CreditAttribution: tkoleary commentedSeconded. Fantastic work.
Comment #120
joelpittetCreated a followup for an icon that is stretched in FF 24:
#2094817: Vertical toolbar down icons are stretched.
Comment #120.0
joelpittetUpdated issue summary.