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

Attach metadata to icons #9122

Closed
callumbwhyte opened this issue Oct 8, 2020 · 14 comments
Closed

Attach metadata to icons #9122

callumbwhyte opened this issue Oct 8, 2020 · 14 comments
Labels
state/needs-investigation This requires input from HQ or community to proceed status/stale Marked as stale due to inactivity type/feature

Comments

@callumbwhyte
Copy link
Contributor

Following a demo of the new <umb-icon> directive by @MMasey (looks awesome!) a discussion started about how we can make the experience of working with icons nicer.

It was discussed that it would be great to attach additional metadata around icons. This could be used to displaying a label alongside the icon, or improving search within the icon picker.

I think it would be cool to define icons as JSON, rather than files. This way we could store additional metadata.

[
    {
        "alias": "cat",
        "path": "umbraco/icons/cat.svg",
        "group": "animals"
    }
]

The alias field is the current name (minus -icon). The path is where the image is located (this could be relative to the /umbraco folder or absolute for packages, as is the case elsewhere in the CMS).

The group field is a proposal for a better way of grouping icons, perhaps to improve the currently overwhelming icon picker with a grouped experience much like the data type picker.

The Umbraco would have a big JSON file defining icons in this way. Following along the lines of #8884 the package.manifest file could also offer a way to define / override custom icons in the same way for package developers.

It's important that any labels or user-facing text can be localised - currently the icon picker search is based on icon CSS class / file names, which are in English and don't accurately represent the icon or are misspelled. The JSON structure doesn't lend itself too well to localisation, but the existing XML dictionary files could handle this by convention:

<area alias="icons">
    <key alias="cat">Cat</key>
</area>

This is just a dump of ideas in the hope we can agree on an approach and split this into some digestible chunks!

@skttl
Copy link
Contributor

skttl commented Oct 12, 2020

Just thinking out loud, how about just adding this meta data as data-attributes in the svg file itself?

<svg xmlns="http://www.w3.org/2000/svg" 
    viewBox="0 0 512 512" 
    data-umb-alias="yen-bag" 
    data-umb-category="money"
    data-umb-keywords="yen money currency china">
    <path
        d="M324.816 172.772l.014.027c-.08-.108-.146-.213-.242-.319a810.557 810.557 0 00-12.461-16.761c-5.395-9.025-8.242-20.006-1.619-35.849v-.009l22.776-54.505-35.808-16.298 3.696-16.156h-89.834l4.708 20.585-36.804 11.869 22.761 54.505.018.009c11.247 26.945-4.856 39.813-14.325 52.928-46.861 64.796-99.313 157.564-99.313 213.49 0 92.734 72.008 89.646 167.857 89.646 95.881 0 167.889 3.088 167.889-89.646 0-55.934-52.467-148.702-99.313-213.516zm-91.138-45.504h45.879v28.931h-45.879v-28.931zm73.431 179.369v20.791h-31.283l-3.282 6.344v10.058h34.565v20.782h-34.565v30.181h-33.245v-30.181h-34.336V343.83h34.336v-10.058l-3.041-6.344h-31.295v-20.791h20.801l-33.489-68.015h35.449l28.859 68.015 28.221-68.015h35.435l-33.915 68.015h20.785z" />
</svg>

@callumbwhyte
Copy link
Contributor Author

callumbwhyte commented Oct 12, 2020

@skttl Quite smart, someone else proposed similar when I discussed this too. There's definitely pro's for having the data stored here, such as more efficient data lookups.

My fear is this data is a little inaccessible to contributors, and a JSON / config would feel more familiar - having everything in one place might make things a bit clearer. Perhaps also a little "mysterious" for package devs adding custom icons, who may not know about needing to add non-standard attributes to the SVG files as text for it to work.

Your proposal of also attaching keywords is interesting, something I also considered in my initial proposal but excluded for one important reason... I'm unsure how best to localize this data. A Danish user in the CMS won't naturally search these terms to find an icon, for example. Having the data in the SVG doesn't help with this, but I'm not sure the current XML dictionary is the best place either... Perhaps something by convention here?

@skttl
Copy link
Contributor

skttl commented Oct 12, 2020

Could the alias be inferred from the filename if not present in an umb-alias attribute? And have a default general category for alle icons without a defined category?

I think having to accompany icons with a json object will make it harder, and more subject to error, than just dumping svg's in a folder. I would also find the json object equally mysterious as the attributes.

@skttl
Copy link
Contributor

skttl commented Oct 12, 2020

For localization we could use the same strategy as in examine.
data-umb-keywords for invariant keywords
data-umb-keywords_da-DK for danish keywords
data-umb-keywords_en-UK for english keywords

@nul800sebastiaan
Copy link
Member

I'm also a big fan of putting the data in the actual svg file, we can always document where to change it.
Having to load a json config file and the actual SVG seems a bit more overhead than is needed.

However, we don't want the localization to be inside the file since that is just going to bloat the file and you'll only use 1 localization typically, no need to keep the rest of the strings in memory!

Note: we have the feature at the moment to put a lang folder in the ~App_Plugins/[MyPlugin]/lang folder. Maybe make localization of the icons an optional install? And make it so that people can pick applicable languages to load for icons? I find it a bit difficult to imagine that many people will care too much about searching for winkelwagen instead of cart when picking an icon for their doctype.
This lack of imagination, obviously, is because up until today we've not had the option to even translate these icon names at all 😅

@nul800sebastiaan nul800sebastiaan added state/needs-investigation This requires input from HQ or community to proceed type/feature labels Oct 14, 2020
@callumbwhyte
Copy link
Contributor Author

Agree that localised strings shouldn't be in the file... But not entirely sure about your proposal with the lang folder? I think localisation and even labels would be entirely optional, with fallbacks to the icon alias as currently.

I feel I'm personally in the JSON file camp, just because it feels more logical / familiar and easier to contribute to - surely we're going to have to cast this data into some kind of object in the backend anyway... I'd be keen to see what performance different (if any) there would be with both approaches too!

(No way is "cart" "winkelwagen" in Dutch!?! 😂)

@nathanwoulfe
Copy link
Contributor

Has the reverse been considered - SVG strings in the JSON? Would result in a larger JSON file, but would then be a single request to get all icons.

Could be handled like the language XML and loaded into the browser on startup, as a collection of all discovered icons (ie core ones and any added by packages etc)

@callumbwhyte
Copy link
Contributor Author

@nathanwoulfe You raise an interesting point about caching / performance...

Currently an SVG can be loaded infinite times during the session by the <umb-icon> directive. Having all of these pre-loaded would at least avoid the backend lookups... I know that @filipbech had some ideas about caching these after first load with service workers...

One hit to the disk and caching everything will certainly be quicker than reading 600 individual files and parsing the data, though the payload will be big and if you only need ~10/600 icons it's rather wasteful... The compromise in my view is storing the paths / data in the browser, much like languages dictionaries - a smaller footprint initially and resulting SVGs could be cached in the browser afterwards.

I think the JSON could get bloated very quickly and overwhelming to contribute to etc if the SVG was in there too, but perhaps an option to add the data inline is good in some cases: We could include an "element" property in the object which falls back to "path" or "url" if missing...

@nathanwoulfe
Copy link
Contributor

That sounds like a sensible middle ground - could pre-prime too with the icons required by the backoffice, to remove that hit.

Would be possible too to return multiple icons in a single request, to speed things up more.

@bjarnef
Copy link
Contributor

bjarnef commented Nov 8, 2020

I like the feature to reference the SVG to a Group, which is similar to how property editors belong to a group (maybe "common" by default?).

With this the icon picker could have a dropdown to filter icons by group/category.

One thing we also need to consider is a paged result of the icons. At the moment in v8.9.0 if having e.g. 2000+ SVG icons in /Umbraco/assets/icons folder it is causing memory issues in the browser when filtering the results as all icons is rendered in the DOM and the ngRepeat items are filtered. I have submitted a separate feature request for this here: #9352

@bjarnef
Copy link
Contributor

bjarnef commented Nov 9, 2020

Regarding searching icons, maybe inspiration from this could be useful: https://itsjavi.com/fontawesome-iconpicker/

[
    {
        "alias": "cat",
        "path": "umbraco/icons/cat.svg",
        "group": "animals",
        "searchTerms": ['kitten', 'pet']
    }
]

The searchTerms property could also be named tags if it makes more sense.

I think it would be most simple to reference, but maybe the option to include the SVG in JSON instead, but it might be too cluttered as the SVG could a large (although it probably should be simple to work as icon).

Not sure if it would be possible to create a SVG sprite sheet with the icons.
https://css-tricks.com/svg-sprites-use-better-icon-fonts/

Then it would be easy to reference the specific SVG icon. However I am not sure how it would work if a cat.svg exists in multiple locations.

<svg viewBox="0 0 100 100" class="icon-cat">
  <use xlink:href="#icon-cat"></use>
</svg>

@MMasey
Copy link
Contributor

MMasey commented Nov 9, 2020

I like the format that @bjarnef has suggested, but I do think it needs to remain a path to the SVG rather than in the JSON itself, otherwise it would potentially cause a breaking change with the IconPickerController. That being said, maybe there could be a difference field for an inline icon?

In regards to using an SVG sprite, I did consider this as an option when initially creating the umb-icon PR, but the issue with the approach is that it would add a huge amount of data to the DOM considering by default there are 600+ icons, and with the ability to add more this can easily go over 1000. It also limits some of the potentially functionality a full inline SVG could have. I do wonder though whether that svg sprite sheet could be added to dynamically though, which would be a fair amount of work, but could be a good approach. It would mean it wouldn't be as easy to animate any of the svg icons, but that may be a non-issue.

@bjarnef
Copy link
Contributor

bjarnef commented Nov 9, 2020

@MMasey for some project I am using gulp-svg-sprite to generate a SVG sprite sheet https://una.im/svg-icons/ and gulp-cheerio to optimize the SVG further.
Not sure if a similar approach can be used to generate a SVG sprite on fly though.

@umbrabot
Copy link

Hiya @callumbwhyte,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant
This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@umbrabot umbrabot added the status/stale Marked as stale due to inactivity label Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/needs-investigation This requires input from HQ or community to proceed status/stale Marked as stale due to inactivity type/feature
Projects
None yet
Development

No branches or pull requests

8 participants