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

[Joy] Apply color inversion to components #34602

Merged
merged 294 commits into from Dec 23, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Oct 4, 2022

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 hook useColorInversion 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 add context to the color prop.
  • classes: add colorContext class
  • test: add describeJoyColorInversion conformance

@siriwatknp siriwatknp added the package: joy-ui Specific to @mui/joy label Oct 4, 2022
@mui-bot
Copy link

mui-bot commented Oct 4, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-34602--material-ui.netlify.app/

@mui/joy: parsed: +0.61% , gzip: +0.70%

Details of bundle changes

Generated by 🚫 dangerJS against f11c411

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 10, 2022
@siriwatknp siriwatknp changed the title [Joy] Apply variant inversion to components [Joy] Apply color inversion to components Oct 11, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

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:

image

test/utils/describeJoyColorInversion.tsx Show resolved Hide resolved
packages/mui-joy/src/styles/defaultTheme.ts Show resolved Hide resolved
@@ -51,6 +51,18 @@ describe('<Tooltip />', () => {
}),
);

describeJoyColorInversion(
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

packages/mui-joy/src/Textarea/Textarea.tsx Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

@mnajdova I have added visual regression for all components (including the portal slots) and added more tests as suggested.

@siriwatknp
Copy link
Member Author

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:

image

I don't understand your point on this, can you elaborate?

@mnajdova
Copy link
Member

I don't understand your point on this, can you elaborate?

I just meant that the blue icons look weird on the red background :)

Copy link
Member

@mnajdova mnajdova left a 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.

Off topic: we probably want to improve the scrollbar styles
image

@siriwatknp
Copy link
Member Author

siriwatknp commented Dec 15, 2022

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

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 image

Nice catch, using color-scheme: dark should be the best solution (just for solid variant).

@siriwatknp
Copy link
Member Author

@mnajdova Added color-scheme: dark for the solid variant except for warning color.

Screen.Recording.2565-12-15.at.13.14.25.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>

@mnajdova
Copy link
Member

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?

@siriwatknp
Copy link
Member Author

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?

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.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 19, 2022
@siriwatknp
Copy link
Member Author

@oliviertassinari @danilo-leal @michaldudak Any thought around #34602 (comment)?

@mnajdova
Copy link
Member

Let's keep it as is for now, we can see if there are valid use-cases of my point from above.

Copy link
Member

@mnajdova mnajdova left a 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 ;)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 23, 2022
@siriwatknp siriwatknp merged commit 68ef353 into mui:master Dec 23, 2022
@michaldudak
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants