-
Notifications
You must be signed in to change notification settings - Fork 71
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
Created Modus Icon component and added icons for expand all and collapse all #1673
Conversation
✅ Deploy Preview for modus-webcomponents ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
LGTM, but you should get approval from owners of the modus-web-components before merging.
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.
@rjavier-trimbler, For every pull request, please ensure it is associated with a GitHub issue. Could you kindly create the issue and provide a link?
References #1098 |
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.
@rjavier-trimbler @jkendell89 Your contribution is greatly appreciated! Thank you. I have added comments, and please let me know if you have any questions.
stencil-workspace/storybook/stories/components/modus-icons/modus-icons.stories.tsx
Show resolved
Hide resolved
stencil-workspace/src/components/icons/icon-chevron-double-down.tsx
Outdated
Show resolved
Hide resolved
stencil-workspace/storybook/stories/components/modus-icons/modus-icons-storybook-docs.mdx
Outdated
Show resolved
Hide resolved
stencil-workspace/storybook/stories/components/modus-icons/modus-icons-storybook-docs.mdx
Show resolved
Hide resolved
@rjavier-trimbler @jkendell89 Could you please create a new GitHub issue? Modus needs more details for this issue for tracking purposes, and the existing Issue #1098 is a bigger story that might not be appropriate. |
stencil-workspace/storybook/stories/components/modus-icons/modus-icons-storybook-docs.mdx
Outdated
Show resolved
Hide resolved
stencil-workspace/storybook/stories/components/modus-icons/modus-icons-storybook-docs.mdx
Outdated
Show resolved
Hide resolved
@rjavier-trimbler Thank you for fixing other icons like the calendar icon. A couple of comments are pending. |
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.
Whichever icon is updated, kindly make sure the default colour as currentcolor
not the hex code.
stencil-workspace/src/components/icons/icon-chevron-double-down.tsx
Outdated
Show resolved
Hide resolved
stencil-workspace/src/components/icons/icon-chevron-double-up.tsx
Outdated
Show resolved
Hide resolved
stencil-workspace/src/components/icons/icon-chevron-down-thick.tsx
Outdated
Show resolved
Hide resolved
stencil-workspace/src/components/icons/icon-chevron-up-thick.tsx
Outdated
Show resolved
Hide resolved
@Prop() size?: string = '16'; | ||
|
||
/** (optional) The color of the Icon */ | ||
@Prop() color?: string = '#6A6976'; |
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.
Let's not have a default hex code for now. Instead, please update each of the above icons to use the default as currentcolor
for fill
Great PR - thanks for the contribution! |
Linking this PR to an issue |
Description
References #
https://e-builderinc.atlassian.net/browse/NAV-227
Type of change
How Has This Been Tested?
Checklist