-
Notifications
You must be signed in to change notification settings - Fork 315
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!: <ThemeProvider> propagate classname to div wrapping children, instead of children directly #1870
Conversation
…instead of passing it to children directly
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.
just mentioning this is a breaking change so it should be done carefully together with the rest of the changes
… sergeyro/feature/theme-provider-propagate-classname-to-div
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.
test is failing
</>, | ||
<> | ||
ThemeProvider is populating theme name <code>className</code> to {"it's child, so don't put "} | ||
<code>{"<Fragment>"}</code> (<code>{"<>"}</code>) inside - {` it's not accepting `} | ||
<code>className</code> prop. | ||
</> |
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.
maybe replace it with notice about div
src/components/ThemeProvider/__tests__/themeProvider-tests.jest.js
Outdated
Show resolved
Hide resolved
…propagate-classname-to-div' into sergeyro/feature/theme-provider-propagate-classname-to-div
… sergeyro/feature/theme-provider-propagate-classname-to-div
bcc384e
into
sergeyro/feature/theme-provider-rename-theme-prop
https://monday.monday.com/boards/3532714909/pulses/5729775904
Targetting this PR #1869
READY TO MERGE