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
[Joy] Apply color inversion to components #34602
Conversation
@mui/joy: parsed: +0.61% , gzip: +0.70% |
a4476c0
to
f16b27e
Compare
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.
@@ -51,6 +51,18 @@ describe('<Tooltip />', () => { | |||
}), | |||
); | |||
|
|||
describeJoyColorInversion( |
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.
We should ensure that without disablePortal
the context color won't be applied.
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.
👍
@mnajdova I have added visual regression for all components (including the portal slots) and added more tests as suggested. |
I just meant that the blue icons look weird on the red background :) |
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.
One last concern I have is requiring disablePortal
to be set so that the color inversion can work. I understand this motivation from an implementation point of view, but I don't think it's the best DX. We should be able to make it work without requiring this prop in my opinion. We can anyway add additional prop that can alter this behavior if you think there is need for it.
I try to avoid adding more props for this. There are some use cases where color inversion does not need to be enabled by default for popups. This is why I think color inversion does not need to be enabled by default for popups. I feel that this is an acceptable exception for popups to have Nice catch, using |
@mnajdova Added Screen.Recording.2565-12-15.at.13.14.25.movOut of scope: In the future, I plan to expose style utilities that will make the scrollbar looks better with the color inversion feature. import { getScrollbarStyles } from '@mui/joy/styles';
<Sheet invertedColors ...>
<Autocomplete
items={[...]}
slotProps={{ listbox: { sx: { ...getScrollbarStyles() } } }}
/>
</Sheet> |
…variant-inversion
Probably there was a misunderstanding. I totally agree that there are use-cases, my opinion was that disabling portals and disabling color inversion are two different concepts, which in my opinion shouldn't be correlated. I would rather have a prop like: |
Got you point, but it would make it more confusing in this case: // The color inversion should work but it is not.
<Menu disablePortal={false} disablePopupColorInversion={false}> and also in the case that default // default `disablePortal` is false, default `disablePopupColorInversion` is false
// The color inversion does not work unless `disablePortal` is explicitly set to `true`.
<Menu> At the end, developers have to learn that |
@oliviertassinari @danilo-leal @michaldudak Any thought around #34602 (comment)? |
Let's keep it as is for now, we can see if there are valid use-cases of my point from above. |
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.
Nice, let's 🚢 it ;)
…variant-inversion
Sorry for coming in late. |
Preview: https://deploy-preview-34602--material-ui.netlify.app/joy-ui/main-features/color-inversion/
Changes
Continuing from #32511, this PR applies color inversion to all components. There are 4 parts:
getColor
from the internal hookuseColorInversion
to get the final color value. For more details, take a look at "How it works".ApplyColorInversion
on the component's ownerState types to addcontext
to the color prop.colorContext
classdescribeJoyColorInversion
conformance