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

[Input] Support CSS variables #32128

Merged
merged 13 commits into from
May 3, 2022

Conversation

@mui-bot
Copy link

mui-bot commented Apr 3, 2022

Details of bundle changes

@material-ui/core: parsed: +0.15% , gzip: +0.16%

Generated by 🚫 dangerJS against 0c88491

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.

Thanks for looking into this :) I've left two comments. As this is still very early in the development there will be occurrences like this where we will need to make a decision, so sorry if this takes a bit longer :)

Comment on lines 45 to 46
const light = (theme.vars || theme).palette.mode === 'light';
const bottomLineColor = light ? 'rgba(0, 0, 0, 0.42)' : 'rgba(255, 255, 255, 0.7)';
Copy link
Member

@mnajdova mnajdova Apr 4, 2022

Choose a reason for hiding this comment

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

This goes against the purpose of using the new CSS variables structure, as we would still be depending on JS transformations. My proposal would be creating new theme token in the palette something like inputBorderColor that will have the correct values in the light and dark mode. We would add this only in the experimental_extendTheme default theme, to avoid changes in the current theme structure.

@danilo-leal danilo-leal changed the title [Input] Support css variables [Input] Support CSS variables Apr 4, 2022
@danilo-leal danilo-leal added component: text field This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 4, 2022
@siriwatknp
Copy link
Member

@mnajdova I have to add a few more variables to make it works.

  • I moved opacity inside colorSchemes because I think it is quite common that the opacity can be different between color schemes (the placeholder is the real example).
  • Added placeholder and inputTouchBottomLine to opacity
    {
      active: 0.54,
      hover: 0.04,
      selected: 0.08,
      disabled: 0.26,
      focus: 0.12,
    +placeholder,
    +inputTouchBottomLine,
    }
  • Refactor the extendTheme function to be more readable (reduce variable creation and use more mutation).
  • Add more tests

@mnajdova
Copy link
Member

@mnajdova I have to add a few more variables to make it works.

I moved opacity inside colorSchemes because I think it is quite common that the opacity can be different between color schemes (the placeholder is the real example).
Added placeholder and inputTouchBottomLine to opacity

{
  active: 0.54,
  hover: 0.04,
  selected: 0.08,
  disabled: 0.26,
  focus: 0.12,
+ placeholder,
+ inputTouchBottomLine,
}

Exactlly what I had in mind 👍

@@ -1,7 +1,6 @@
import { deepmerge } from '@mui/utils';
import { colorChannel } from '@mui/system';
import createThemeWithoutVars from './createTheme';
import createPalette from './createPalette';

export const defaultOpacity = {
Copy link
Member

Choose a reason for hiding this comment

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

@siriwatknp I think we need to have different default opacity values for light and dark theme to simplify the implementaiton. Now we have different values for inputTouchBottomLine and placeholder, but in MD even the hover opacity should have different value in light vs dark mode.

Copy link
Member

Choose a reason for hiding this comment

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

This difference is the reason why in the Button.js I am using the opacity from the action.hoverOpacity.

Copy link
Member

@siriwatknp siriwatknp Apr 29, 2022

Choose a reason for hiding this comment

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

@mnajdova I think we should not add the active, disabled ... to the theme.vars.opacity at this point because we already have theme.vars.palette.action.xxxOpacity which are already configurable on both light and dark color schemes.

Adding theme to theme.vars.opacity could create confusion and we are not using them in the migration at all. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's skip them for now. Long term I think we should have all opacities under one object, that was my intention with the comment.

@@ -33,6 +33,7 @@ export interface TypeAction {
export interface TypeBackground {
default: string;
paper: string;
invertChannel: string;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's safe to add this type here. The createTheme is not adding this value. I think we can just cast to this in the extendTheme util if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, sounds good to me.

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.

The changes look good to me. @siriwatknp please see one last comment related to the types updates in createPalette.d.ts

@siriwatknp siriwatknp merged commit 0956b91 into mui:master May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants