Skip to content
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

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

jamesrkiger
Copy link
Contributor

@jamesrkiger jamesrkiger commented Jan 8, 2025

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

📣 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:

  • archive/share/delete options in project list view
  • edit/delete options in user permissions section of sharing view
  • member actions dropdown trigger in organizations member table

@jamesrkiger jamesrkiger changed the title Add ActionIcon theming and story feat(organizations): add theming/story for Mantine ActionIcon TASK-1448 Jan 8, 2025
@jamesrkiger jamesrkiger self-assigned this Jan 8, 2025
@jamesrkiger jamesrkiger marked this pull request as draft January 8, 2025 20:05
@jamesrkiger jamesrkiger marked this pull request as ready for review January 9, 2025 12:12
'--ai-hover': theme.colors.blue[5],
}),
...(props.variant === 'light' && {
'--ai-color': theme.colors.blue[5],
Copy link
Member

@magicznyleszek magicznyleszek Jan 10, 2025

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)

Copy link
Contributor

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).

Copy link
Contributor Author

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.

jsapp/js/components/common/actionIcon.stories.tsx Outdated Show resolved Hide resolved
@jamesrkiger
Copy link
Contributor Author

@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?

Base automatically changed from kalvis/mantine-setup to main January 15, 2025 08:50
@Akuukis Akuukis requested a review from p2edwards as a code owner January 15, 2025 08:50
@jamesrkiger jamesrkiger force-pushed the task-1448-mantine-action-icon branch from 084ae8e to 0484f21 Compare January 15, 2025 12:22
Copy link
Contributor

@pauloamorimbr pauloamorimbr left a 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.

jsapp/js/components/common/ActionIcon.tsx Outdated Show resolved Hide resolved
@jamesrkiger jamesrkiger merged commit e424952 into main Jan 20, 2025
7 checks passed
@jamesrkiger jamesrkiger deleted the task-1448-mantine-action-icon branch January 20, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants