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

[GridList] Deprecate, rename ImageList #14994

Closed
wants to merge 2 commits into from

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 21, 2019

Deprecation

The material design specification has renamed the component one year ago:

-import GridList from '@material-ui/core/GridList';
-import GridListTile from '@material-ui/core/GridListTile';
-import GridListTileBar from '@material-ui/core/GridListTileBar';
+import ImageList from '@material-ui/core/ImageList';
+import ImageListTile from '@material-ui/core/ImageListTile';
+import ImageListTileBar from '@material-ui/core/ImageListTileBar';

@oliviertassinari oliviertassinari added the component: image list This is the name of the generic UI component, not the React module! label Mar 21, 2019
@oliviertassinari oliviertassinari force-pushed the image-list branch 2 times, most recently from 1824b8a to 13058c8 Compare March 21, 2019 14:31
@eps1lon eps1lon mentioned this pull request Mar 21, 2019
56 tasks
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 21, 2019

@material-ui/core: parsed: +0.26% , gzip: +0.11%

Details of bundle changes.

Comparing: f9896bc...f5e4a65

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.26% 🔺 +0.11% 🔺 352,709 353,623 90,549 90,646
@material-ui/core/Paper +0.01% 🔺 0.00% 68,626 68,632 19,973 19,973
@material-ui/core/Paper.esm 0.00% 0.00% 62,358 62,358 19,073 19,073
@material-ui/core/Popper 0.00% 0.00% 30,460 30,460 10,528 10,528
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,384 17,384 5,726 5,726
@material-ui/core/useMediaQuery 0.00% -0.19% 2,469 2,469 1,038 1,036
@material-ui/lab 0.00% +0.01% 🔺 152,181 152,186 44,528 44,534
@material-ui/styles 0.00% 0.00% 53,837 53,837 15,615 15,615
@material-ui/system 0.00% -0.11% 17,138 17,138 4,522 4,517
Button 0.00% 0.00% 90,918 90,918 26,969 26,969
Modal 0.00% 0.00% 84,712 84,712 25,262 25,262
colorManipulator 0.00% 0.00% 3,232 3,232 1,300 1,300
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main 0.00% 0.00% 647,785 647,785 201,132 201,132
packages/material-ui/build/umd/material-ui.production.min.js +0.06% 🔺 +0.04% 🔺 301,799 301,983 83,742 83,778

Generated by 🚫 dangerJS against f5e4a65

@oliviertassinari
Copy link
Member Author

@eps1lon I'm wondering about the breaking change vs deprecation. It should be straightforward to migrate, plus this component is rarely used.

@eps1lon
Copy link
Member

eps1lon commented Mar 21, 2019

@eps1lon I'm wondering about the breaking change vs deprecation. It should be straightforward to migrate, plus this component is rarely used.

That's not what SemVer is concerned with. It makes no assumptions about how "straightforward" the maintainer thinks it is. The consumer has to take actions since the public API changed and it is therefore a breaking change.

We could just export GridList from ImageList and add a warning in __DEV__. If you think that is to much effort for too little gain we could make a breaking change without deprecation. That's not a bad thing.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 21, 2019

If you think that is to much effort for too little gain

@eps1lon Yes, it's what I was wondering about going for a deprecation vs a breaking change strategy. Given how the changes are structured right now, it's a BC. But, we can change it, 🤔.

@mbrookes
Copy link
Member

Deprecation vs. breaking change is about more than semver too. It's also about DX:

  • Breaking change without deprecation: I've upgraded, something isn't working, and I need to figure out why.
  • Deprecation: I've upgraded, I'm getting a warning, and I know what I need to do to fix it.

@oliviertassinari
Copy link
Member Author

OK, I'm gonna make it a deprecation.

@oliviertassinari oliviertassinari added deprecation New deprecation message and removed breaking change labels Mar 22, 2019
@eps1lon
Copy link
Member

eps1lon commented Mar 22, 2019

Deprecation vs. breaking change is about more than semver too. It's also about DX

Semver is about the DX. It has no intention to provide version strings for anything else than developers. It's one of the reason why people often criticize it because it can't be used to communicate "big" or "small", "interesting" or "important" changes. Semver never attempted to solve this. This is a concern of marketing.

Personally I wouldn't even deprecate it in v4. Deprecations should happen in a minor before a breaking version (semver recommendation). If this doesn't happen we might as well make the cut now. Otherwise we should defer to 4.1. Personally a deprecation during a breaking change will look equivalent to a breaking change.

@oliviertassinari oliviertassinari added this to the v4.1 milestone Mar 22, 2019
@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Mar 22, 2019
@oliviertassinari oliviertassinari changed the title [GridList] Rename ImageList [GridList] Deprecate, rename ImageList Mar 22, 2019
@oliviertassinari
Copy link
Member Author

I'm closing for the moment. I will come back to it later, probably in 4-6 months to prepare v5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: image list This is the name of the generic UI component, not the React module! deprecation New deprecation message on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants