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(Mantine): support mantine 7 #6345

Open
wants to merge 60 commits into
base: main
Choose a base branch
from
Open

Conversation

alodela
Copy link

@alodela alodela commented Sep 17, 2024

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

glebtv and others added 30 commits February 21, 2024 14:53
@BatuhanW BatuhanW changed the base branch from master to releases/october October 7, 2024 12:24
@alodela
Copy link
Author

alodela commented Oct 8, 2024

this is great, how can we starting building with this branch @alodela 🙏

@hanza93 you can install @refinedev/mantine from my forked repository. Also, make sure to use the latest versions of the rest of the @refinedev/* packages. Here's what I'm doing to build from my branch:

"@refinedev/mantine": "github:alodela/refine#path:packages/mantine&mantine7.v4",

Just consider this PR is still under review and might change.

@aliemir aliemir deleted the branch refinedev:main October 14, 2024 12:54
@aliemir aliemir closed this Oct 14, 2024
@aliemir aliemir reopened this Oct 14, 2024
@aliemir aliemir changed the base branch from releases/october to master October 14, 2024 12:56
@@ -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}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the justify="space-between" attribute is missing (replaces position="apart"):

<Group justify="space-between" {...headerProps}>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

With the v7 upgrade it looks like a primary button:
grafik

With varlian="subtle" it looks similar to the v5 version:
grafik

Works also with dark mode (see https://mantine.dev/core/action-icon/#usage).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kruschid Great suggestion. I've updated all the back buttons.

variant: mapButtonVariantToActionIconVariant(variant),
}
: { variant: "default" })}
variant={mapButtonVariantToActionIconVariant(variant, "default")}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 = (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kruschid Thanks for pointing this out. I've removed the mapButtonVariantToActionIconVariant function from the ActionIcon.

@BatuhanW
Copy link
Member

BatuhanW commented Nov 8, 2024

Hey @kruschid sorry for the late response, we've been busy through the week. We'll answer your comments next week.

@kruschid
Copy link

kruschid commented Nov 8, 2024

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

@BatuhanW
Copy link
Member

Hey @alodela Would you like to release Mantine 7 support as a standalone package on npm and your own repository? We'd be more than happy to update documentation and website, mention that Mantine 7 supported library is yours and add links to your package.

@alodela
Copy link
Author

alodela commented Nov 24, 2024

That's a generous offer, @BatuhanW, but I'm a bit worried about keeping the standalone Mantine package up and running in the long run since bandwidth can be an issue. What’s the thinking behind releasing Mantine support as a standalone package? I would love to hear your thoughts!

@aliemir
Copy link
Member

aliemir commented Nov 25, 2024

I'm hoping to deliver a detailed review of the PR very soon. Thank you for your work, @alodela! 🙌

For now and in the foreseeable future, our focus will primarily be on delivering new features and improvements to Refine's core. As a result, we find it challenging to keep up with major releases from external dependencies like Mantine.

However, we remain fully committed to supporting community-driven initiatives like this. If you decide to take ownership, we’ll promote the package alongside the ones we officially release and offer any help needed for maintenance and up-keeping.

Thanks again for your hard work and dedication. I'll post my review in following days 🙏

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for refine-doc-live-previews ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 58cf201
🔍 Latest deploy log https://app.netlify.com/sites/refine-doc-live-previews/deploys/675a4708f2cb660008752c09
😎 Deploy Preview https://deploy-preview-6345--refine-doc-live-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

[FEAT] Mantine v7 support?
6 participants