-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: added theme support for icons #975
base: main
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for harness-design failed.
|
✅ Deploy Preview for harness-xd-review ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
82e535d
to
e0f602c
Compare
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.
most of the images which need a light version, are not even "icons" strictly.
i think we need to discuss and come up with a better approach for handling this.
packages/ui/src/components/icon.tsx
Outdated
@@ -102,15 +101,30 @@ import Italicize from '../icons/italicize.svg' | |||
import Key from '../icons/key-icon.svg' | |||
import Lightning from '../icons/lightning.svg' | |||
import List from '../icons/list.svg' | |||
import CreateWorkspaceLight from '../icons/lists-data-icons/create-workspace-light.svg' |
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.
this is not a good/scalable way to support "light" versions of icons as it just results in proliferation by doubling almost all icons.
we can probably do something similar to how we handled it earlier: https://github.com/harness/uicore/blob/main/packages/icons/src/Icon.tsx
basically we added an inverse
prop on the Icon
component, which looks for a specific file name pattern for the selected icon. if it's available the inverted one is returned, if not, the default one is returned.
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.
@abhinavrastogi-harness Got it. So in our case, I think we should also check if there is an icon with the -light
postfix, and use it; otherwise, we fallback to the default one.
But in fact, in the example, icons for inverse are added in the same way, just with a fallback. Correct?
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.
@abhinavrastogi-harness I have made the changes to the PR, you can review it
da15041
to
7875dad
Compare
ce7b0d7
to
3b63c4f
Compare
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 we can pretty drastically cut down on this changeset by requiring that consuming apps wrap their entire app in the ThemeProvider
. We can then look for the theme directly from the Icon
component and not need to alter any other view or component or require the consumer pass additional props.
Regarding different SVGs for light vs dark icons, I'm not really convinced. From experience, the majority of icons should just be invertible to get the same effect without additional weight. It might be better to add a CSS invert filter when the theme is light. In some cases this won't be ideal, so we can add in selective overrides; the flow in that case would be that we get the normal icon name, check if the theme is light, if so check if there is a light version, if so use it, otherwise use the normal icon with the invert filter.
Worth noting that work is also underway to tokenise the strokes/fills etc of icons, so in the future we'll be able to set the colouring/thickness/etc of SVG icons to whatever we need it to be for the theme from tokens studio. We're not there yet so the override/invert is probably a good stopgap.
apps/design-system/src/subjects/views/repo-empty/repo-empty-view.tsx
Outdated
Show resolved
Hide resolved
1f4bc13
to
4d8877d
Compare
4d8877d
to
1c44c11
Compare
97d852c
to
617b6a5
Compare
ThemeProvider
context to UIuseThemeStore
atdesign-system