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

fix: mantine css leak #157

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

fix: mantine css leak #157

wants to merge 2 commits into from

Conversation

Elvincth
Copy link

This pull request fixes the issue of CSS leakage from the Mantine component library by scoping its styles within a specific root element.

Details:

We used the postcss-prefix-selector plugin to add a prefix (#jotai-devtools-root) to all Mantine styles. This ensures that Mantine styles are contained and do not affect other parts of the application.

Result of generated index.css:
image

Testing Instructions:

Test the fix in this repository: Jotai Devtool Test CSS Leak. Use pnpm link to link the dev package.

Thank you!

Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@arjunvegda arjunvegda left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this!

@@ -1,5 +1,15 @@
module.exports = {
plugins: {
'postcss-preset-mantine': {},
'postcss-prefix-selector': {
Copy link
Member

Choose a reason for hiding this comment

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

Were we able to figure out the issues with data-mantine-* attribute?

Copy link
Author

Choose a reason for hiding this comment

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

Currently this plugin can't fix the data-mantine-* attribute issue, as I searched the mantine source code data-mantine-color-scheme are hardcoded in some parts

Copy link
Author

Choose a reason for hiding this comment

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

For this issue I have opened a discussion in https://github.com/orgs/mantinedev/discussions/6573

Copy link
Author

Choose a reason for hiding this comment

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

@arjunvegda What do u think if we merge this fix first and provide a fix for data-mantine-* attribute issue later?

Copy link
Member

Choose a reason for hiding this comment

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

Does the dark/light mode + everything else work as expected with this change?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the sentiment, we should look into migrating away from Mantine. ShadCN is a good alternative, would anyone like to give it a shot? Ideally, we'd want to have 1:1 migration (with theme, look, etc.) with zero impact on the userland.

Copy link

Choose a reason for hiding this comment

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

Not sure if we even need tailwind (not that I'm against using it, or other CSS libraries) -- as long as the devtools can be styled with dark and light theme support, it should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

But using shadcn can reduce the overhead of migration. A great thing is that it is built on top of Radix, meaning it’s unstyled, so breaking changes are less likely to occur.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with the sentiment, we should look into migrating away from Mantine. ShadCN is a good alternative, would anyone like to give it a shot? Ideally, we'd want to have 1:1 migration (with theme, look, etc.) with zero impact on the userland.

Actually, I am interested in the migration task as I would like to make the same kind of devtool for zustand.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I've been a little occupied lately. Let me DM you later this week @Elvincth

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.

3 participants