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.

Sign up for GitHub

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

Jump to bottom

[ButtonGroup] Add support for CSS variables #32498

Merged
merged 8 commits into from
May 6, 2022
Merged

[ButtonGroup] Add support for CSS variables #32498

merged 8 commits into from
May 6, 2022

Conversation

vicasas
Copy link
Member

@vicasas vicasas commented Apr 28, 2022 •
edited
Loading

@vicasas vicasas added new feature New feature or request component: ButtonGroup The React component. labels Apr 28, 2022
@vicasas vicasas requested review from mnajdova and siriwatknp April 28, 2022 22:48
@mui-bot
Copy link

mui-bot commented Apr 28, 2022 •
edited
Loading

Details of bundle changes

Generated by 🚫 dangerJS against cb66b2a

@vicasas
Copy link
Member Author

vicasas commented Apr 29, 2022

theme.palette.mode should not be changed to (theme.vars || theme).palette.mode? I ask since I saw in other PRs that it was maintained. Is this for something special?

@siriwatknp siriwatknp mentioned this pull request Apr 29, 2022
Closed
65 tasks
@mnajdova mnajdova changed the title [ButtonGroup] Add support CSS variables [ButtonGroup] Add support for CSS variables Apr 29, 2022
@mnajdova
Copy link
Member

theme.palette.mode should not be changed to (theme.vars || theme).palette.mode? I ask since I saw in other PRs that it was maintained. Is this for something special?

I would say we should never care for the mode when using CSS variables, as the values should already be transformed. We should keep it only for JS transformations with the current theme structure. Having this said, there is no need to check it from the vars.

siriwatknp
siriwatknp approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Love it!

siriwatknp
siriwatknp requested changes Apr 29, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

We still need to consider

...(ownerState.variant === 'text' &&
  ownerState.orientation === 'vertical' && {
    borderBottom: `1px solid ${
      theme.palette.mode === 'light' ? 'rgba(0, 0, 0, 0.23)' : 'rgba(255, 255, 255, 0.23)'
    }`,
  }),

@vicasas
Copy link
Member Author

vicasas commented Apr 29, 2022

We still need to consider

Yes, I do take it into account. Something like this quickly occurs to me, I don't know if it will be the approach to follow.

I have doubts with the values ​​for certain things... I'll leave them in [?]

const light = theme.palette.mode === 'light'
let borderBottomColor = light ? 'rgba(0, 0, 0, 0.23)' : 'rgba(255, 255, 255, 0.23)'
if (theme.vars) {
  borderBottom = `rgba([?] / [?])`
}

siriwatknp
siriwatknp reviewed May 5, 2022
View reviewed changes
packages/mui-material/src/ButtonGroup/ButtonGroup.js
Comment on lines +120 to +124
borderBottom: theme.vars
? `1px solid rgba(${theme.vars.palette.common.onBackgroundChannel} / 0.23)`
: `1px solid ${
theme.palette.mode === 'light' ? 'rgba(0, 0, 0, 0.23)' : 'rgba(255, 255, 255, 0.23)'
}`,
Copy link
Member

@siriwatknp siriwatknp May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@vicasas I have added

  • common.backgroundChannel
  • common.onBackgroundChannel (the on* follows Material Design convention)

in #32128.

I think it makes sense to use it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@siriwatknp Perfect! I'll check back later to make the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I misunderstood the comment at first. 😅

backgroundChannel and onBackgroundChannel returns a color for dark/light?

Copy link
Member

Choose a reason for hiding this comment

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

@vicasas yes.

light mode, --md-palette-common-backgroundChannel: 255 255 255;.
dark mode, --md-palette-common-backgroundChannel: 0 0 0;.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! now it makes sense to me

siriwatknp
siriwatknp requested changes May 6, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

wait for the update to use onBackgroundChannel.

siriwatknp
siriwatknp approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

By bad, I looked at the wrong line 😂

@siriwatknp siriwatknp merged commit 51874b0 into mui:master May 6, 2022
@vicasas vicasas deleted the buttongroup-css-vars branch May 6, 2022 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siriwatknp siriwatknp siriwatknp approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees
No one assigned
Labels
component: ButtonGroup The React component. new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vicasas @mui-bot @mnajdova @siriwatknp