-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add Mantine theme #4692
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
Add Mantine theme #4692
Conversation
@Algirdyz Left you some feedback. Can you rebase and make sure that |
Rebased, and updated based on review |
packages/playground/package.json
Outdated
"access": "public" | ||
} | ||
} | ||
} |
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.
Can you restore the blank line at the end of the file?
packages/mantine/package.json
Outdated
"@mantine/core": ">=7", | ||
"@mantine/hooks": ">=7", | ||
"@mantine/dates": ">=7" |
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.
Are Mantine 7 and 8 totally compatible? Is Mantine 7 still supported? We want to reduce the frequency at which we have to publish breaking changes, so with that in mind, would it make sense to drop 7 and just support 8?
@Algirdyz it looks like you forgot to run |
I am only getting this when I run
|
Mantine v8 was fully compatible in this case, just had to switch the version, no code changes required. |
I can see that there is something wrong with the layoutgrid. I can't get the |
Ok fixed layoutgrid and added screenshots, is this what you wanted? |
@heath-freenome Sorry for my late to response on #4560, I was unable to continue working on it due to my personal life commitments @Algirdyz Thank you for continuing this work. If you need it, I am ready to contribute to complete it now. |
@Algirdyz Sorry for the slow reply, busy work week. So I just wanted to see the layout grid screenshot attached as a comment to the PR rather than being code within the PR to make sure it looked right. Feel free to rebase and remove the screenshot from the actual codebase. I didn't mean to confuse you. |
packages/shadcn/package.json
Outdated
"jsdom": "^20.0.3", | ||
"tailwindcss": "^3.0.0" | ||
"tailwindcss": "^3.0.0", | ||
"postcss-load-config": "^6.0.1" |
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.
(Why) is this needed for shadcn? Unexpected in this PR
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.
Hmm, seems to no longer be needed. Previously it would give me error building shadcn saying this package was missing. Ayway, removed it.
@Algirdyz I think you just need to fix the new snapshot test and this is good to merge! |
Co-authored-by: Nick Grosenbacher <[email protected]>
Co-authored-by: Nick Grosenbacher <[email protected]>
Co-authored-by: Nick Grosenbacher <[email protected]>
Co-authored-by: Heath C <[email protected]>
Co-authored-by: Heath C <[email protected]>
- Minor documentation cleanup
Reasons for making this change
Trying to finalize this PR: #4560
If this is related to existing tickets, include links to them as well. Use the syntax
fixes #[issue number]
(ex:fixes #123
).If your PR is non-trivial and you'd like to schedule a synchronous review, please add it to the weekly meeting agenda: https://docs.google.com/document/d/12PjTvv21k6LIky6bNQVnsplMLLnmEuypTLQF8a-8Wss/edit
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.