feat(Mantine): support mantine 7 #6345
Conversation
…ontext from unthemed layout
Co-authored-by: Alican Erdurmaz <[email protected]>
|
Hey @alodela, thank you for your effort, great job! (Also thanks to @glebtv). 🚀 🚀 About the change from emotion to CSS Modules, I'm not sure if this requires a big change for I'll try to review all files as soon as possible. Of course there can be changes in the overall styling and we won't have issues with them. I'll mainly check for components to make sure we're not missing any features we're trying to keep all UI packages aligned with. 🙏 |
|
Hi @aliemir, I kept the note about the Mantine CSS module from the previous PR, just in case. So far, I haven't seen any issues. The Mantine CSS modules can be included in the Refine app, and as you said, use the I fixed the issues in the examples, and hopefully, everything will pass the checks next time. I'll be keeping an eye on the PR for any comments. Thanks. |
|
this is great, how can we starting building with this branch @alodela 🙏 |
@hanza93 you can install "@refinedev/mantine": "github:alodela/refine#path:packages/mantine&mantine7.v4",Just consider this PR is still under review and might change. |
| @@ -111,17 +111,13 @@ export const Create: React.FC<CreateProps> = (props) => { | |||
| return ( | |||
| <Card p="md" {...wrapperProps}> | |||
| <LoadingOverlay visible={loadingOverlayVisible} /> | |||
| <Group position="apart" align="center" {...headerProps}> | |||
| <Stack spacing="xs"> | |||
| <Group {...headerProps}> | |||
There was a problem hiding this comment.
here the justify="space-between" attribute is missing (replaces position="apart"):
<Group justify="space-between" {...headerProps}>There was a problem hiding this comment.
@kruschid Thanks for catching this. I added the justify attribute.
| @@ -161,7 +161,7 @@ export const Edit: React.FC<EditProps> = (props) => { | |||
| ); | |||
|
|
|||
| const buttonBack = | |||
| goBackFromProps === (false || null) ? null : ( | |||
| goBackFromProps === false || goBackFromProps === null ? null : ( | |||
| <ActionIcon | |||
There was a problem hiding this comment.
I'd suggest adding the variant="subtle" attribute to all the ActionIcon components that are used for the back buttons in the crud components.
This is the style of the action icon in v5:
With the v7 upgrade it looks like a primary button:
With varlian="subtle" it looks similar to the v5 version:
Works also with dark mode (see https://mantine.dev/core/action-icon/#usage).
There was a problem hiding this comment.
@kruschid Great suggestion. I've updated all the back buttons.
| variant: mapButtonVariantToActionIconVariant(variant), | ||
| } | ||
| : { variant: "default" })} | ||
| variant={mapButtonVariantToActionIconVariant(variant, "default")} |
There was a problem hiding this comment.
The mapButtonVariantToActionIconVariant(variant, "default") call appears to be unnecessary, as it essentially variant={variant || "default"}. This applies to all other buttons (create, delete, ..., save, show) too. If my understanding is correct, the entire package/mantine/src/definitions folder could be removed.
There was a problem hiding this comment.
@kruschid Totally agree! The mapButtonVariantToActionIconVariant function really doesn’t seem needed in Mantine v7.
| @@ -2,11 +2,13 @@ import type { ActionIconVariant, ButtonVariant } from "@mantine/core"; | |||
|
|
|||
| export const mapButtonVariantToActionIconVariant = ( | |||
There was a problem hiding this comment.
In Mantine v5 the variant "white" was mapped to "default" because Mantine v5 doesn't implement a "white" variant (see https://v5.mantine.dev/core/action-icon/#usage). However, this is not the case in v7.
As a result, it's difficult to understand the motivation behind keeping the mapButtonVariantToActionIconVariant function.
Especially if we consider that in every call, both arguments variant and defaultVariant are always defined (I checked each call in the button components), so the default value "default" is never returned. It seems that variant || defaultVariant as a button attribute value could be a replacement for this function, what do you think?
There was a problem hiding this comment.
@kruschid Thanks for pointing this out. I've removed the mapButtonVariantToActionIconVariant function from the ActionIcon.
|
Hey @kruschid sorry for the late response, we've been busy through the week. We'll answer your comments next week. |
That's okay, it's quite a large PR. I'm trying to upgrade my project to v7 using the changes here. I couldn't test every detail yet, but still wanted to contribute, at least by adding some comments. @glebtv and @alodela have done good work here 🚀 , and my comments are rather cosmetic. If I find more issues, I'll make further comments. Thank you. 👍 |
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
Support for Mantine 5
What is the new behaviour?
Continuing the work done in #5340 to support Mantine 7
Notes for reviewers
Notable problems:
Mantine 7 uses CSS modules for styling, and @refinedev/mantine uses tsup to build, which doesn't support css modules
egoist/tsup#536
Any input or feedback welcome
Mantine migration guides:
https://v6.mantine.dev/changelog/6-0-0/
https://mantine.dev/guides/6x-to-7x/
closes #5178