liferay / clay

A web implementation of the Lexicon Experience Language
http://clayui.com
Other
208 stars 467 forks source link

feat(@clayui/css): LPD-4556 Convert SVG icons to 16x16 #5810

Open pat270 opened 2 months ago

pat270 commented 2 months ago

https://liferay.atlassian.net/browse/LPD-4556

I'm marking this as draft because we have some inconsistencies in naming, missing 16x16 icons, and questions about what to do with deprecated icons. The table below shows the naming conflicts Lexicon has with Clay. Lexicon needs to rename them so it matches Clay due to backward compatibility issues. I'm good with Lexicon prefixing with icon- but the rest of the name should match.

Lexicon should rename to match Clay Clay icon names
icon-document-vectorial.svg document-vector.svg
icon-field.svg Doesn't exist in Clay
icon-info-circle-full.svg info-circle.svg ?
icon-info-circle-line.svg info-circle-open.svg ?
icon-info-circle-open.svg info-panel-open.svg ?
icon-lock-wireless.svg Doesn't exist in Clay
icon-oauth2.svg oauth.svg
icon-openID open-id.svg
icon-page-tree.svg pages-tree.svg
icon-paste-plain-text.svg paste-plaintext.svg
icon-rule-builder.svg should be rule-builder.svg was added as icon-rule-builder.svg
icon-square-hole-multiple.svg square-hole-multi.svg
icon-thumb-up-arrow.svg thumbs-up-arrow.svg
icon-twitter-x.svg Should be social-twitter-x.svg, there was a mistake and we didn't prefix it

This next table lists icons that are in Clay but doesn't exist in the Lexicon Figma Library. They haven't been deprecated.

Clay icons that aren't in Lexicon Comments
add-role.svg
anonymize.svg
bell-full.svg
change-list-disabled.svg
comments.svg
custom-field.svg
devices.svg
diagnol-line.svg
environment-connected.svg
environment-disconnected.svg
environment.svg
flags-fr-ca.svg This is the Canadian flag, should we duplicate it in Lexicon?
flags-sr-RS-latin.svg Should we duplicate sr-RS flag in Lexicon?
flags-ta-IN.svg Should be duplicate hi-IN in Lexicon?
info-circle.svg
info-panel-closed.svg
info-panel-open.svg
megaphone-full.svg
minus-circle.svg
pin-full.svg
remove-role.svg
reset.svg
share-alt.svg
sign-in.svg
social-twitter.svg
sticky.svg
third-party.svg

Lastly, what should we do about deprecated icons? My gut says it's dangerous to remove them, but I'd like opinions on this.

Deprecated icons in Clay Status Replacement Comments
announcement.svg deprecated megaphone-full.svg We need megaphone-full.svg from Lexicon
desktop.svg deprecated display-content.svg No action needs to be taken
import-export.svg deprecated order-arrow.svg No action needs to be taken
embed.svg deprecated code.svg No action needs to be taken
social-twitter.svg active Should be deprecated? If not we need a 16x16 version from Lexicon
sticky.svg deprecated pin-full.svg We need pin-full.svg from Lexicon
twitter.svg deprecated social-twitter.svg We need social-twitter.svg from Lexicon
urgent.svg deprecated bell-full.svg We need bell-full.svg from Lexicon

/cc @matuzalemsteles @ethib137 @marcoscv-work

matuzalemsteles commented 2 months ago

For the deprecated icons I would say that we can still keep them and remove them in the future major version, it seems safer, we can now add a warning in the documentation for the deprecated icons.

For Lexicon I'm not sure if it needs to update the icon names especially if it's just the file names, I think the designers just use the names in Figma now if the Figma name is different from what exists in Clay I think we can discuss and understand the best option here, mainly because the nomenclature in Clay or Lexicon can be strange so we need to discuss and reach a common sense what we should do about the names you listed.

For these icons that don't exist in Lexicon, maybe because they were removed or the name is different?

pat270 commented 2 months ago

For the deprecated icons I would say that we can still keep them and remove them in the future major version, it seems safer, we can now add a warning in the documentation for the deprecated icons.

The problem is the deprecated icons are 512 x 512. Lexicon will need to create these icons in 16x16.

For these icons that don't exist in Lexicon, maybe because they were removed or the name is different?

I checked and they're not in the Figma.

One thing I forgot the mention is it's pretty easy to convert 16 x 16 to 512 x 512 from the Figma file. Maybe we don't need to make the 16x16 change.

matuzalemsteles commented 2 months ago

The problem is the deprecated icons are 512 x 512. Lexicon will need to create these icons in 16x16.

Hmm I think we can convert these icons to 16x16 just by changing the size I think there shouldn't be any problems especially if the icons and borders are drawn with just the path instead of the figma resource so decreasing the icon in proportion should work fine.

One thing I forgot the mention is it's pretty easy to convert 16 x 16 to 512 x 512 from the Figma file. Maybe we don't need to make the 16x16 change.

I don't remember exactly why but why do we want to have 16x16 icons?

pat270 commented 2 months ago

I don't remember exactly why but why do we want to have 16x16 icons?

@matuzalemsteles the icons in the Figma Library is 16x16. It's to keep things consistent. The only problem I run into is exporting the whole library. As of now, I need to change the width and height of the icon in the Figma and export them individually. It's not a problem if there's only a few icons we add each time.

pat270 commented 2 months ago

@matuzalemsteles I dug a little bit deeper in Portal and it looks like we can interchange 16x16 and 512x512. I'll update this pr and test it.

pat270 commented 2 months ago

This is ready for review. I was able to test in most places and it seems to work fine regardless of having viewBox="0 0 16 16" or viewBox="0 0 512 512". I also tested icons served from CDN. We should probably remove https://github.com/liferay/liferay-portal/blob/3dfc363015a7a315d3bc7688dd78179a41171c95/modules/apps/frontend-taglib/frontend-taglib-clay/src/main/java/com/liferay/frontend/taglib/clay/servlet/taglib/IconTag.java#L31.

drakonux commented 2 months ago

Let me review all your comments, @matuzalemsteles. It is a huge task, and without Emi, I'll need to take my time.

matuzalemsteles commented 2 months ago

@drakonux no problem, don't worry!

pat270 commented 1 month ago

@matuzalemsteles @drakonux I reverted the problem icons mentioned above on my local branch. There are no more off center issues. We just need to work on the discrepancies between strokes and icon sizes.