Skip to content

[SwitchBase] Replace IconButton with ButtonBase#26460

Merged
siriwatknp merged 9 commits into
mui:nextfrom
siriwatknp:fix/switch-base-inheritance
Jun 1, 2021
Merged

[SwitchBase] Replace IconButton with ButtonBase#26460
siriwatknp merged 9 commits into
mui:nextfrom
siriwatknp:fix/switch-base-inheritance

Conversation

@siriwatknp

@siriwatknp siriwatknp commented May 26, 2021 •
edited by oliviertassinari
Loading

Copy link
Copy Markdown
Member

closes #21503, #23945

the changes affect these components

  • Switch
  • Checkbox
  • Radio

The final UI looks the same but I consider this as breaking change because of html change.

Before

  • Switch => SwitchBase => IconButton => ButtonBase
  • Checkbox => SwitchBase => IconButton => ButtonBase
  • Radio => SwitchBase => IconButton => ButtonBase

SwitchBase is internal component, can not be theme.

<IconButton root>
  <IconButton label>
    <SwitchBase>

New

  • Switch => SwitchBase => ButtonBase
  • Checkbox => SwitchBase => ButtonBase
  • Radio => SwitchBase => ButtonBase
<ButtonBase>
  <SwitchBase>

There is no label from IconButton anymore, so 1 level of DOM reduced.

Preview: https://deploy-preview-26460--material-ui.netlify.app/components/switches/

@siriwatknp siriwatknp added breaking change Introduces changes that are not backward compatible. scope: checkbox Changes related to the checkbox. scope: switch Changes related to the switch. scope: radio Changes related to the radio. labels May 26, 2021
@mui-pr-bot

mui-pr-bot commented May 26, 2021 •
edited
Loading

Copy link
Copy Markdown

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.12% , gzip: +0.09%

Generated by 🚫 dangerJS against 916f702

@github-actions github-actions Bot mentioned this pull request May 26, 2021
Closed
@siriwatknp
siriwatknp force-pushed the fix/switch-base-inheritance branch from 0ad225a to eb2a388 Compare May 26, 2021 09:04
@oliviertassinari

oliviertassinari commented May 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Note that #26303 works on a close problem.

@siriwatknp siriwatknp mentioned this pull request May 26, 2021
Closed
1 task
@DanielBretzigheimer

DanielBretzigheimer commented May 26, 2021

Copy link
Copy Markdown
Contributor

Looks like a more simple solution with less breaking changes compared to #26303. But as you said, the structure has one theoretically unnecessary DOM element (ButtonBase).

One thing that's missing in this PR is the focus indicator.

@siriwatknp

siriwatknp commented May 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Looks like a more simple solution with less breaking changes compared to #26303. But as you said, the structure has one theoretically unnecessary DOM element (ButtonBase).

One thing that's missing in this PR is the focus indicator.

Are you saying that the span.ButtonBase-root should be removed?

image

Should it be

<span class="Switch-root">
  <span class="Switch-input">
  <span class="Switch-thumb">
  <span class="Switch-track">
</span>

@DanielBretzigheimer

DanielBretzigheimer commented May 27, 2021

Copy link
Copy Markdown
Contributor

Are you saying that the span.ButtonBase-root should be removed?
When only looking for the DOM structure → Yes, I think this would be the ideal solution.

But as I have seen in the PR #26303 this causes many other problems and redundant code. So my conclusion is to stick with the ButtonBase to improve maintainability and keep the changes as few as possible. All the Issues should be solved with this solution.

But keep in mind, that currently the focus indicator is not working. If you want I will look into it :)

@siriwatknp

siriwatknp commented May 27, 2021

Copy link
Copy Markdown
Member Author

But as I have seen in the PR #26303 this causes many other problems and redundant code. So my conclusion is to stick with the ButtonBase to improve maintainability and keep the changes as few as possible. All the Issues should be solved with this solution.

But keep in mind, that currently the focus indicator is not working. If you want I will look into it :)

Sounds good!

mnajdova
mnajdova reviewed May 28, 2021
View reviewed changes
Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated

### Checkbox

- The className does not have `.MuiIconButton-root` and `.MuiIconButton-label` anymore, target `.MuiButtonBase-root` instead.

@mnajdova mnajdova May 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would propose showing a diff with the changes.

mnajdova
mnajdova reviewed May 28, 2021
View reviewed changes
Comment thread packages/material-ui/src/internal/SwitchBase.js
/* Styles applied to the root element. */
padding: 9,
});
borderRadius: '50%',

@mnajdova mnajdova May 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's funny that this was the only thing required from the IconButton :)

mnajdova
mnajdova reviewed May 28, 2021
View reviewed changes

@mnajdova mnajdova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would propose going with this set of changes at the start. It's clear improvement, and it is much easier to follow what is changed. We can then on top of these changes try to apply the changes from #26303

@siriwatknp
siriwatknp requested a review from mnajdova May 31, 2021 02:40
mnajdova
mnajdova reviewed May 31, 2021
View reviewed changes
Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated
mnajdova
mnajdova reviewed May 31, 2021
View reviewed changes
Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated
mnajdova
mnajdova reviewed May 31, 2021
View reviewed changes
Comment thread docs/src/pages/guides/migration-v4/migration-v4.md Outdated
mnajdova
mnajdova approved these changes May 31, 2021
View reviewed changes

@mnajdova mnajdova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've pushed minor changes on the migration-v4.md, looks good otherwise.

@siriwatknp
siriwatknp merged commit 8fc684f into mui:next Jun 1, 2021
@siriwatknp
siriwatknp deleted the fix/switch-base-inheritance branch June 1, 2021 03:26
@oliviertassinari oliviertassinari changed the title [SwitchBase] replace IconButton with ButtonBase [SwitchBase] Replace IconButton with ButtonBase Jun 6, 2021
@oliviertassinari oliviertassinari mentioned this pull request Jun 6, 2021
Merged
1 task
@oliviertassinari

oliviertassinari commented Jun 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

closes #21503, #23945

@siriwatknp Didn't you mean to close the second issue too? You needed to add the magic keyword for each issue.

This was referenced Jun 6, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mnajdova mnajdova mnajdova approved these changes

Assignees

No one assigned

Labels

breaking change Introduces changes that are not backward compatible. scope: checkbox Changes related to the checkbox. scope: radio Changes related to the radio. scope: switch Changes related to the switch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't override IconButton styles in theme without affecting Checkbox and Switch

5 participants

@siriwatknp @mui-pr-bot @oliviertassinari @DanielBretzigheimer @mnajdova