search

trimble-oss / modus-web-components

This library provides Modus components as web components - reusable, encapsulated UI elements that are framework agnostic (can be implemented in any site).
https://modus-web-components.trimble.com/
MIT License
31 stars 65 forks source link

Default icon color inconsistencies #1889

Open ryan-henness-trimble opened 7 months ago

ryan-henness-trimble commented 7 months ago

Prerequisites

Describe the issue

There are some inconsistencies in the default icon colors that have made it harder to keep behavior consistent across all icons. The inconsistencies are:

  1. Which elements the fill color property values are set on. Some set the fill on the svg, and others on the path elements.
  2. Which colors are used as the default values for those properties. Some use 'currentColor', others use #6A6976 or #252a2e

To resolve these issue and improve icon consistency, I suggest a few changes:

  1. Use the fill property on the svg elements inside of the icon components, rather than the path elements
  2. Default the fill property value to 'currentColor' - which means that the fill color of that path will be the same as the color property of the element or its parent element (see example: https://jsfiddle.net/s4c1e2nu/1/)
  3. Use a constant value for the icon default size of 16

Reduced test cases

No response

What operating system(s) are you seeing the problem on?

No response

What browser(s) are you seeing the problem on?

No response

What is the issue regarding ?

@trimble-oss/modus-web-components

What version of npm package are you using ?

Latest

Priority

Medium

What product/project are you using Modus Components for ?

Work Center

What is your team/division name ?

Trimble Viewpoint

Are you willing to contribute ?

Yes

Are you using Modus Web Components in production ?

No response

github-actions[bot] commented 7 months ago

Hello @ryan-henness-trimble! Thanks for opening an issue. The Modus core team will get back to you soon (usually within 24-hours) and provide guidance on how to proceed. Contributors are welcome to participate in the discussion and provide their input on how to best solve the issue, and even submit a PR if they want to.

Please wait until the issue is ready to be worked on before submitting a PR, or you can reach out to the core team if it is time bound. For trivial things, or bugs that don't change the expected behaviors and UI, you can go ahead and make a PR.

ryan-henness-trimble commented 7 months ago

@coliff @msankaran0712 one quick question about the review process - will one of you reach out once this issue has been reviewed by your team?

rthanga1 commented 7 months ago

@ryan-henness-trimble We normally review the issues every Friday and we did refined this today and we are taking it

coliff commented 6 months ago

some rules/changes I have in mind:

I'm unsure about changing the default icon size to make them all 16x16 as it could have unintentional side effects if the icons are in use on sites.

rthanga1 commented 5 months ago

@coliff/ @ryan-henness-trimble Can you confirm if this is fixed by Ryan's latest PR?

cjwinsor commented 2 months ago

@coliff Can you validate this ticket is resolved, I think your #1946 does what is mentioned above. Or resolved by the new method for including icons (or at least not broken again by that)