-
-
Notifications
You must be signed in to change notification settings - Fork 185
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(organizations): add theming/story for Mantine ActionIcon TASK-1448 #5412
Conversation
jsapp/js/theme/kobo/ActionIcon.ts
Outdated
'--ai-hover': theme.colors.blue[5], | ||
}), | ||
...(props.variant === 'light' && { | ||
'--ai-color': theme.colors.blue[5], |
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 that the blue color of the icon is lighter than our current icon buttons.
Also, is there a reason why these overrides doesn't happen at the styles file? I think that modifying styles through vars
is not the approch we want to go (see https://github.com/kobotoolbox/kpi/pull/5366/files)
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 prefer CSS Modules file over styles prop, because that edits CSS class once, instead of adding
style="font-size: calc(0.875rem * var(--mantine-scale)); border-top: 1px solid rgb(237, 238, 242);"
to every cell individually (see DOM of storybook). Also, that's officially recommended.Furthermore, would you agree to prefer vars prop over CSS Modules file, because that's a simple parameterization of the CSS Modules? Although performance is unknown and official docs lacks an opinion on using vars prop vs "hardcoded" values in CSS Modules.
Preferring vars over css is an option, let's talk in the meetup about our policy (options: prefer vars, prefer css, undefined).
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 that the blue color of the icon is lighter than our current icon buttons.
Good catch. I have fixed this for the 'light' and 'danger-secondary' variants.
@magicznyleszek I have updated this PR to include a wrapped customization of the ActionIcon component that takes an icon name as a prop and automatically includes the appropriate sized icon. Would you mind taking another look? |
084ae8e
to
0484f21
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.
LGTM!
Left 1 simple comment with an opinion, but it's up to you.
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's global📣 Summary
Adds theming and storybook entry for a custom ActionIcon component that automatically includes an appropriate sized icon.
💭 Notes
We previously used our custom in-house button component to display icon buttons. With Mantine, however, these kinds of buttons are rendered with the ActionIcon component.
👀 Preview steps
Use storybook stories and compare to existing icon buttons in the repo. Existing examples include: