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

[ExpansionPanel] Rename to Accordion #21494

Merged
merged 42 commits into from
Jun 23, 2020
Merged

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jun 18, 2020

Breaking changes

  • [ExpensionPanel] Rename the ExpansionPanel components with Accordion to match the naming convention of the community:

    -import ExpansionPanel from '@material-ui/core/ExpansionPanel';
    -import ExpansionPanelSummary from '@material-ui/core/ExpansionPanelSummary';
    -import ExpansionPanelDetails from '@material-ui/core/ExpansionPanelDetails';
    -import ExpansionPanelActions from '@material-ui/core/ExpansionPanelActions';
    +import Accordion from '@material-ui/core/Accordion';
    +import AccordionSummary from '@material-ui/core/AccordionSummary';
    +import AccordionDetails from '@material-ui/core/AccordionDetails';
    +import AccordionActions from '@material-ui/core/AccordionActions';  
    
    -<ExpansionPanel>
    +<Accordion>
    -  <ExpansionPanelSummary>  
    +  <AccordionSummary>
         <Typography>Location</Typography>
         <Typography>Select trip destination</Typography>
    -  </ExpansionPanelSummary>  
    +  </AccordionSummary>
    -  <ExpansionPanelDetails>
    +  <AccordionDetails>
         <Chip label="Barbados" onDelete={() => {}} />
         <Typography variant="caption">Select your destination of choice</Typography>
    -  </ExpansionPanelDetails>  
    +  </AccordionDetails>
       <Divider />
    -  <ExpansionPanelActions>  
    +  <AccordionActions>
         <Button size="small">Cancel</Button>
         <Button size="small" color="primary">Save</Button>
    -  </ExpansionPanelActions>   
    +  </AccordionActions>
    -</ExpansionPanel>
    +</Accordion>

This PR is inspired by @oliviertassinari comment #20580 (comment) for renaming the ExpansionPanel* components to Accordion*.

I think that we should rename the component in v5 ExpansionPanel -> Accordion. The pros:

Help users migrating from Bootstrap to Material-UI: https://getbootstrap.com/docs/4.4/components/collapse/#accordion-example.
Help users already familiar with a11y components: https://www.w3.org/TR/wai-aria-practices-1.1/examples/accordion/accordion.html.
Break free from MD, help with future themes
The query has, at least, x5 more SEO traffic.

TODOs

  • add migration entry
  • add codemods?

Questions

  • What do we do with the translations?
  • Should we start preparing codemods for v5 (material-ui-codemod/src/v5.0.0)

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 18, 2020

@material-ui/core: parsed: +0.05% , gzip: +0.12%
@material-ui/lab: parsed: 0.00% 😍, gzip: +0.35%

Details of bundle changes

Generated by 🚫 dangerJS against f034f65

@mnajdova
Copy link
Member Author

image

How should we fix the codemod tests for v4?

@mnajdova mnajdova marked this pull request as ready for review June 18, 2020 08:05
@eps1lon
Copy link
Member

eps1lon commented Jun 18, 2020

Ideally the codemod would support the old and new names. If that's not possible then we can always only handle the new names and release the codemod in a major.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Makes sense now that the material guidelines no longer describe this component.

Though we don't want to loose the legacy ExpansionPanel traffic. We should add the moved pages to docs/public/_redirects (301)

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

For the URL, I think that we should continue with the singular: https://material-ui.com/components/accordion/ as we do with the new components in the lab.

docs/translations/translations-aa.json Outdated Show resolved Hide resolved
mnajdova and others added 2 commits June 18, 2020 12:48
…components.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 18, 2020
@mnajdova mnajdova removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 18, 2020
@mbrookes
Copy link
Member

What's the deprecation strategy for ExpansionPanel? Reexport with a warning, then remove in v6?

@mbrookes
Copy link
Member

mbrookes commented Jun 22, 2020

@mnajdova

I probably misunderstood the comments

I didn't help matters by asking a leading question. Sorry!

@eps1lon

Rather we work exclusively on breaking changes and backport it as non-breaking change later onto master.

Am I understanding correctly?:

  1. Rename ExpansionPanel to Accordion on the next branch as a breaking change. (What @mnajdova had done originally.)
  2. Later, backport the rename to master, reexporting Accordion as ExpansionPanel with a deprecation warning. (Essentially what this PR now covers now, but without the docs changes?)

@eps1lon
Copy link
Member

eps1lon commented Jun 23, 2020

Am I understanding correctly?:

Yes! The idea is generally to avoid:

  • wasted effort if a breaking change is later reverted
  • make the PR more focused
    A breaking change can already spark a lot of discussion. It's definitely useful to include a deprecation strategy in the discussion but an outline should be enough in my opinion.
  • free up mental capacity for the current change so that you don't have to worry about breaking changes. This hopefully makes for a "fresher look" at the problem by getting rid of all the legacy baggage.

There's a concern that we might forget to deprecations later (as with any change that is scheduled for later). My hope is that it's far less likely since we can later look at all the breaking changes (filter by label) and see if we have a deprecation in place.

@mnajdova
Copy link
Member Author

@eps1lon so should we revert the changes which are refering to the deprecation of the ExpansionPanel components from this PR?

@eps1lon
Copy link
Member

eps1lon commented Jun 23, 2020

@eps1lon so should we revert the changes which are refering to the deprecation of the ExpansionPanel components from this PR?

At this point that's even more work. This is good to go. Just something to consider in the future.

Just to make sure: In its current state this PR isn't breaking (ignoring dev-only warnings). I can still apply this change to existing codebases and have no change in production?

@mnajdova
Copy link
Member Author

Just to make sure: In its current state this PR isn't breaking (ignoring dev-only warnings). I can still apply this change to existing codebases and have no change in production?

Exactly. We still have the previous folder structure for preserving the imports and all components and their typings are re-exported by their old names.

If there is anything else that should be done that I am not aware of, let me know. If there isn't then this PR is non-breaking.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Some of the markdown files are not considered as moved in git e.g. docs/src/pages/components/accordion/accordion.md (while -aa is correctly marked as moved). This can become problematic when looking through the history.

Most of the time git automatically figures this out. For all the other cases you need to explicitly git mv. I guess you need revert these moves and then git mv instead of mv those files. This might be difficult to get right but may be invalueable later!

docs/public/_redirects Show resolved Hide resolved
@mnajdova
Copy link
Member Author

mnajdova commented Jun 23, 2020

Some of the markdown files are not considered as moved in git e.g. docs/src/pages/components/accordion/accordion.md (while -aa is correctly marked as moved). This can become problematic when looking through the history.

Most of the time git automatically figures this out. For all the other cases you need to explicitly git mv. I guess you need revert these moves and then git mv instead of mv those files. This might be difficult to get right but may be invalueable later!

@eps1lon git mv works as expected (shows the file as renamed), see commit 13c6553 but then after I edit the file, the commit looks good: 957fef9 but the whole PR automatically again shows the old one as deleted, and the other file as new one. And then gitk shows me only the history after the renaming :\ But's it definitely strange why it works with some files, and not with others.. Is there some other way of how we can fix this?

Final note: Everything was done in separate commit, but still the end result is unexpected..

@mnajdova mnajdova requested a review from eps1lon June 23, 2020 13:31
@eps1lon
Copy link
Member

eps1lon commented Jun 23, 2020

Tried but it failed ultimately. Didn't find confirmation that renaming is even a separate thing in git or if it derives this from commits. Probably too much effort for marginal gain long term. We don't trace markdown very often anyway.

@mnajdova mnajdova merged commit 2ddc4ce into mui:next Jun 23, 2020
mnajdova added a commit to mnajdova/material-ui that referenced this pull request Jun 24, 2020
* wip

* wip

* reverted some changes

* sorting

* migration

* fix in migration

* fix in migration

* fix in migration

* Update docs/src/pages/getting-started/supported-components/supported-components.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* added redirect

* fixed material design links

* codemod fix

* prettier + formatted

* changed accordions to accordion

* docs:api

* renamed accordions to accordion

* Update docs/src/pages/components/accordion/accordion.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* Update docs/src/pages/components/accordion/accordion.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* Update docs/src/pages/components/accordion/accordion.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* added deprecated exports for ExpansionPanel components

* added motivation for renaming component

* Update docs/src/pages/components/accordion/accordion.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* Update docs/src/pages/guides/migration-v4/migration-v4.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* Update docs/src/pages/guides/migration-v4/migration-v4.md

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

* cleandup migration

* prettier

* changed comments on deprecated components

* removed

* reverted del files

* renamed

* fixed d.ts fiels

* r

* added

* renamed

* renamed types

* redirects

* Revert markdown source changes

* Move markdown source from expansion-panel to accordion

* Expansion Panel -> Accordion

* Revert attempt at renaming

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
@eps1lon eps1lon added this to the v5 milestone Jul 20, 2020
@eps1lon eps1lon mentioned this pull request Aug 5, 2020
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: accordion This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants