material-ui
material-ui copied to clipboard
[Joy] Apply color inversion to components
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:
-
implementation: use the
getColor
from the internal hookuseColorInversion
to get the final color value. For more details, take a look at "How it works". -
typings: use
ApplyColorInversion
on the component's ownerState types to addcontext
to the color prop. -
classes: add
colorContext
class -
test: add
describeJoyColorInversion
conformance
- [x] I have followed (at least) the PR section of the contributing guide.
Messages | |
---|---|
:book: | Netlify deploy preview: https://deploy-preview-34602--material-ui.netlify.app/ |
@mui/joy: parsed: +0.61% , gzip: +0.70%
Generated by :no_entry_sign: dangerJS against f11c4114bc460a2dd0eef741e26e740d0a50f910
@samuelsycamore Hey, Sam. Could you take a look at the overall explanation of https://deploy-preview-34602--material-ui.netlify.app/joy-ui/main-features/color-inversion/? Is there anything missing or unclear?
I will refine the writing again before asking for your final review. Just want to make sure that this draft is good enough to move forward.
The content looks great so far!
@mnajdova I have added visual regression for all components (including the portal slots) and added more tests as suggested.
Can we use on the demos icons that would change their color based on the current color? For example, this looks strange in my opinion:
I don't understand your point on this, can you elaborate?
I don't understand your point on this, can you elaborate?
I just meant that the blue icons look weird on the red background :)
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.
![Screen Shot 2565-12-15 at 12 38 59](https://user-images.githubusercontent.com/18292247/207781318-a51feb00-a85a-45a9-85a5-5192751c456b.png)
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 disablePortal
set to true. All ears to other suggestion.
Off topic: we probably want to improve the scrollbar styles
Nice catch, using color-scheme: dark
should be the best solution (just for solid
variant).
@mnajdova Added color-scheme: dark
for the solid variant except for warning
color.
https://user-images.githubusercontent.com/18292247/207786412-dc6a2b5f-af4f-4125-a1f7-e5a0a20aa7b8.mov
Out 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>
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 disablePortal set to true. All ears to other suggestion.
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: disablePopupColorInversion
, that would disable/enable the feature on the popup regardless of whether the portal is enabled or disabled. I may be overcomplicating things here, so let's hear at least another opinion :) cc @michaldudak what are your thoughts on this? Should the disablePortal
decide whether the color inversion should apply on the popups (menu, select etc.), or should we add a new prop for this?
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 disablePortal set to true. All ears to other suggestion.
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:
disablePopupColorInversion
, that would disable/enable the feature on the popup regardless of whether the portal is enabled or disabled. I may be overcomplicating things here, so let's hear at least another opinion :) cc @michaldudak what are your thoughts on this? Should thedisablePortal
decide whether the color inversion should apply on the popups (menu, select etc.), or should we add a new prop for this?
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 disablePortal
and disablePopupColorInversion
is set to false.
// 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 disablePortal
should be set to true
to make color inversion work so I think adding another prop is excessive.
@oliviertassinari @danilo-leal @michaldudak Any thought around https://github.com/mui/material-ui/pull/34602#issuecomment-1352692921?
Let's keep it as is for now, we can see if there are valid use-cases of my point from above.
cc @michaldudak what are your thoughts on this? Should the disablePortal decide whether the color inversion should apply on the popups (menu, select etc.), or should we add a new prop for this?
Sorry for coming in late. I wonder what's the advantage of using portals in menus by default? If we render the list in-place, we would make this problem less significant. In general, I agree with @mnajdova, that developers shouldn't need to care about our implementation details. If there are differences in behavior or appearance in portal vs in-place modes, we should make it very clear in the docs (and show the steps to make both look the same).