-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
V8: Add umb-icon directive to backoffice to begin transition from icon fonts to svgs #5606
V8: Add umb-icon directive to backoffice to begin transition from icon fonts to svgs #5606
Conversation
This is great, thanks @MMasey ! I think we will need a way to add custom icons, without having to put them in Could be adding icons using a package.manifest. But that is probably another issue. |
@skttl I completely agree. The goal of the PR is to get the svg icon system in, but adding some configuration to place custom svgs wherever you want is definitely something I’d like to seem. It would make it so much easier for package creators for example to add their own icons etc Before something like that is added though I feel like getting the SVG sanitisation set up to remove any script tags from the svg files is important. I have a working solution available, but it relies on the HtmlSanitizer package and I don’t know how to add that to the Umbraco project. |
@MMasey would you need some assistance from HQ? regarding the HtmlSanitizer package? |
@nielslyngsoe Yes please, that would be really helpful. Once it's in there I can get it configured so that you can see how it would be integrated, then we could take it from there? |
This item has been added to our backlog AB#1355 |
# Conflicts: # src/Umbraco.Web.UI.Client/src/less/components/tree/umb-tree.less # src/Umbraco.Web.UI.Client/src/views/components/tree/umb-tree-item.html # src/Umbraco.Web/Editors/BackOfficeServerVariables.cs
After merging @bjarnef's update @BatJan reminded me of the existence of this. I've merged the current v8 into it, but I'm having a bit of trouble figuring out how to do this one: https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Web.UI.Client/src/views/components/tree/umb-tree-item.html#L9 The additional Other than that, if we can strip any Finally, could you have a look at the search icon, it's positioning is a bit off: |
@nul800sebastiaan @MMasey My take on the referenced code block is that the icon needs to be setup using the
|
Thanks @BatJan - I meant to say: that's how I did it now: https://github.com/umbraco/Umbraco-CMS/blob/3316bfd5edcf6d3070ca74d0292a106700b18ed2/src/Umbraco.Web.UI.Client/src/views/components/tree/umb-tree-item.html I had trouble getting the NVDA to speak the expand message, and I don't know how to trigger expansion with my keyboard. Ah.. but it's not in a |
…o keyboard nav works The svg icon should probably be aria-hidden
Alright, keyboard nav works again, but I'm not hearing I also added |
@nul800sebastiaan The missing screen reader announcement might be due to the context, which I can't picture in my mind right now... What I'm saying I'm not sure why it's not happening but it can be related to the context somehow. Personally I would need to take a closer look into it in order to figure out the issue 😃 Actually screen readers can make sense of SVG's if they're using a |
Hey @nul800sebastiaan, thanks for the updates with this 😄 Are you still having issues with implementing the I think for ease of use, maybe having aria-hidden on the SVGs is fine, although we may want to make it a configurable option on the In regards to stripping out any script tags or doing any other sanitation with the SVGs, i could really use some help & guidance from HQ as it would involve adding a new dependency to core. In some other projects I have done I have used https://github.com/mganss/HtmlSanitizer which has worked really well. It allows you to add you own validation rules so I essentially adding all the rules for SVGs, but stripped out the ability to use <script> tags and it works like a charm. I have all of that side of the code pretty much good to go, but I don't know the best approach for including the package into the project as I couldn't find a package.json anywhere, and as I said previously, i didn't want to just through in a new dependency without you all getting a chance to check it out. If there anything else I can help with just let me know. |
# Conflicts: # src/Umbraco.Web.UI.Client/src/views/components/application/umb-navigation.html # src/Umbraco.Web.UI.Client/src/views/components/tree/umb-tree-item.html # src/Umbraco.Web.UI.Client/src/views/logviewer/search.html
# Conflicts: # src/Umbraco.Web.UI.Client/src/views/logviewer/search.html
Alright, we are in a good state now, let's get this merged and we can work on the improvements later! Love the work @MMasey and the feedback everyone! Very much looking forward to shipping this in v8.8.0!! 🎉 |
FYI Moved the labels to the related issue that is closed through this PR! 👍 |
Question. Is this going to be considered a breaking change? There is a lot of CSS changes here, and quite a few have added extra qualifiers to css rules, so things like *[icon-] rules are now much stricter requiring icons to be spans or i elements (which granted most will be, but still, if they have previously been available unqualified, this is now not the case and thus breaking). |
@mattbrailsford Have you tested this at all? I was under the impression that we left all the old icon stuff in place but maybe we can still fix some things up if they didn't need to move in the CSS! An example of what breaks for you would be great here! |
No I haven't but I've just been reviewing the changes. I think for the most part I "follow the rules" of creating icons using The selectors are also not consistent as all allow the If the rule is that icons must be |
Ah cool, it's really hard to anticipate, how people use these things, but I'm sure we can ease up on the strictness! So you're specifically using |
@nul800sebastiaan I'm not myself, but I'm saying "it's not unreasonable to think someone might be" and if changing the specificity of a selector isn't necessary to achieve the end result of the issue (ie, removing the tag requirement would still work), should we really be doing that here. |
Sure thing! I'm guessing @MMasey chose to make them more specific since we only had one type of usage before in Umbraco itself. I didn't think of those scenarios, good catch! |
Hey @nul800sebastiaan & @mattbrailsford. The specificity for the legacy This PR was only the start of what will essentially multiple PRs to eventually go through and replace all old icon font icons with svgs, although we do need to take into account all package creators etc. I really need to pull my finger out an get some docs sorts too to showcase how the I'm happy to go with HQs decision on this, where it's declaring it a breaking change, or adding back more legacy support. |
I’m all for change, I’m just asking for a level of depreciation and procedure like we get in other areas of the codebase so people have a chance to migrate gracefully. CSS currently gets a free pass yet is often the most noticeable type of regression. |
Yeah I completely agree! I’d hate for this PR to have a detrimental impact to existing Umbraco users, especially if it is most likely to affect package creators as working on a fix and releasing means multiple hurdles for end users and developers. I do wonder if the simplest and safest thing to do would be to have another PR that updated the legacy icons so they aren’t restricted? Then once’s we know it’s stable again, we try and document it somewhere to make sure information is visible around how icons work in the Backoffice? I don’t suppose anyone had any examples of packages that this would break? |
So, turns out I did have a few Even worse, these icons only show up under certain conditions (ie, several in date pickers) so would never have spotted them. This is the reason this stuff matters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @mattbrailsford has mentioned we need to ensure that we don't break packages with this change. Therefore I started a PR reverting part of helveticons.less
Any insights into how we can do this the best way are very welcome, thanks in advance.
PR: #8941
span[class^="icon-"], | ||
span[class*=" icon-"] { | ||
display: inline-block; | ||
width: 1em; | ||
height: 1em; | ||
*margin-right: .3em; | ||
|
||
svg { | ||
width: 100%; | ||
height: 100%; | ||
fill: currentColor; | ||
} | ||
|
||
&.large{ | ||
width: 32px; | ||
height: 32px; | ||
} | ||
&.medium{ | ||
width: 24px; | ||
height: 24px; | ||
} | ||
&.small{ | ||
width: 14px; | ||
height: 14px; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MMasey
Can I ask you to describe the intention of this piece?
The reason I'm asking because we are looking into reverting parts of this file, to make it possible to have icons in the same manner as previously. aka. spans and buttons with icon-* class. And by making our icons selectors target everything again, I see that colliding with this. Therefore I would remove this part, but let me know the intention of it, what purpose it serves and then we can figure out how this should existing and whether it should.
Thanks in advance, You can also get in touch via Slack if you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nielslyngsoe, we may have to update the umb-icon
to add an attribute or modifier class so that we can detect when we don't want an icon use use the icon font. Maybe something as simple as adding an icon--svg
when it uses and svg so we can then do something like this
.icon {
// ... icon font code etc
&.icon--svg {
svg {
width: 100%;
height: 100%;
fill: currentColor;
}
&::before {
content: none;
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be cool. Currently, I'm hacking it by looking for spans with icon, but this approach would be way better.
So I'm already working on reverting these parts. But I never finished or tested my work. So if you like please give this a look, and if you like to change something I hope you have access or can make PR towards this one: #8941
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nielslyngsoe, it's used to ensure the svg icons have the correct sizing, as I think when it was icon fonts it was using font sizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, did you see the PR I created for your PR at #8958?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MMasey Thanks, I actually already figured this out, I took this part into the umb-icon.less, I hope this makes sense to you. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @nul800sebastiaan is merging the Revert PR now, but please give it a look to see if you think that can work for now? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nielslyngsoe I've just had another look over your PR at it seems like it has actually remove the functionality to make the svg icons appear in the icon picker.
The revert has renamed getLegacyIcons
back to getIcons
in the iconhelper.service.js
which makes sense, but the other code changes may actually cause issues for backwards compatibility as they were made to help make the fallback work with the umb-icon
, specifically the formatting of the . Also the iconpicker.controller.js
no longer utilises the getAllIcons
method which is responsible to retrieving the svgs.
I think it needs to be something like this (similar to my other PR).
iconHelper.getAllIcons()
.then(icons => {
vm.icons = icons;
vm.loading = false;
iconHelper.getIcons() // Gets legacy icon fronts and appends them to the icon list, removing any duplicates
.then(icons => {
if(icons && icons.length > 0) {
let legacyIcons = icons.filter(icon => !vm.icons.find(x => x.name == icon.name));
vm.icons = legacyIcons.concat(vm.icons);
}
});
});
Also, did the the typo correction for the icon-addressbook.svg
😉
I'm happy to have a call to chat about it if it would be easier that going back and forth via this.
This was initially a much larger PR so I have decided to recreate it with far fewer commits and code changes to hopefully make it easier to merge The existing icon system is still fully functional so once this is in, we could gradually go through the backoffice and make the changes one by one, rather than one huge monolith of a PR (which is what this one ended up being #5293). The majority of the changes that are part of this PR are from adding the 605 new individual svg icons. 😜
Summary
This PR updates almost all of the icons using the icon font found throughout the backoffice with SVGs. Each icon is now it's own svg file with the following naming convention icon-[name]. I have created an IconController that currently pointed to a folder containing these icons and makes them usable via the an angular directive call umb-icon. This not only improves accessibility, but also make it much easier to customise the icons available in the back office.
For this PR I have only updated the IconPicker, and a few other icons that were using the
<ins>
or<span>
tag. The rest of the changes from<i>
to<umb-icon>
can happen later.One thing that I think needs to be added is the ability to sanitise the SVG's as the IconController reads the svg file directly in order for the angular directive to make use of it. I've found a package that can do HTML Sanitizing that can have additional tags and attributes added to the list of allowed data so that it can work with SVGs too. I have this running without issue on a seperate project and can confirm that it works great.
The package is https://github.com/mganss/HtmlSanitizer
I suppose the issue with is it that it has a dependency on AngleSharp, and I know we're trying to reduce dependencies in v8.
<umb-icon />
directiveAll of the individual icon svgs now live under
src\Umbraco.Web.UI.Client\src\assets\icons
. When the frontend build process is run these get copied over to thesrc\Umbraco.Web.UI\Umbraco\assets
folder, in the same way the other frontend source files do.These are 2 API endpoints that can be used to get the SVG markup. The controller for these can be found at
v8\src\Umbraco.Web\Editors\IconController.cs
GetIcon
returns a single IconModel
GetAllIcons
returns List (all icons found in the icons folder, used for the icon picker)
The beauty of this approach is that if you want to add more (non-core) icons, you can add them to the
src\Umbraco.Web.UI\Umbraco\assets
folder and you will have access to them. The means for people creating apps for Umbraco will be able to easily add their own icons. The same goes for those wanting to do other personalisations to the backoffice.In order to see the difference i highly recommend installing the Font Changer with Google Web Fonts Chrome extension and view the icon-font version to get an idea of the problem this solves.
I'd really appreciate some input/feedback whether it's around the coding style, tackling the SVG sanitization (I have a potential solution but don't just want to throw it in there) or just general questions. It would also be sweet to get some help in regards to writing some tests etc.
I've put a fair amount of detail in the issue #3560 so I would recommend having a read through if you'd like some more information.
@nielslyngsoe I know you we're interested in this.
Many thanks
Fixes #3560