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

[SelectInput] Convert to function component #15410

Merged
merged 2 commits into from Apr 21, 2019

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Apr 19, 2019

Works towards #15231

@joshwooding joshwooding added new feature New feature or request component: select This is the name of the generic UI component, not the React module! labels Apr 19, 2019
@joshwooding joshwooding mentioned this pull request Apr 19, 2019
29 tasks
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 19, 2019

@material-ui/core: parsed: -0.26% 😍, gzip: -0.17% 😍

Details of bundle changes.

Comparing: 3044cb4...b175c50

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.26% -0.17% 312,618 311,790 84,447 84,305
@material-ui/core/Paper 0.00% -0.02% 67,279 67,279 19,968 19,965
@material-ui/core/Paper.esm 0.00% 0.00% 60,640 60,640 18,884 18,884
@material-ui/core/Popper 0.00% -0.03% 34,907 34,907 11,812 11,809
@material-ui/core/Textarea 0.00% 0.00% 5,327 5,327 2,291 2,291
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,898 15,898 5,771 5,771
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% -0.00% 141,612 141,612 42,722 42,721
@material-ui/styles 0.00% 0.00% 50,833 50,833 15,013 15,013
@material-ui/system 0.00% 0.00% 11,765 11,765 3,928 3,928
Button 0.00% -0.02% 85,621 85,621 25,588 25,584
Modal 0.00% -0.00% 79,901 79,901 24,011 24,010
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 50,908 50,908 11,210 11,210
docs.main 0.00% 0.00% 648,518 648,518 201,926 201,926
packages/material-ui/build/umd/material-ui.production.min.js -0.28% -0.26% 293,911 293,076 82,492 82,277

Generated by 🚫 dangerJS against b175c50

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.

Awesome work! I have tried something new with this review: 1. Checkout the pull request. 2. Revert the source back to its original state. 3. Make sure the new tests are green.

packages/material-ui/src/Select/SelectInput.test.js Outdated Show resolved Hide resolved
@joshwooding
Copy link
Member Author

Awesome work! I have tried something new with this review: 1. Checkout the pull request. 2. Revert the source back to its original state. 3. Make sure the new tests are green.

I try to do this when re-writing the tests :)

@eps1lon
Copy link
Member

eps1lon commented Apr 20, 2019

Awesome work! I have tried something new with this review: 1. Checkout the pull request. 2. Revert the source back to its original state. 3. Make sure the new tests are green.

This is something I'll try to do with all my PRs: Tests before implementation. This way it's easier for reviewers to verify: is it a bug? would this test new behavior properly etc. Then you can share different implementations based on the same test plan via git branches. If you do impl after test this becomes very weird (hard reset, rebase etc).

@oliviertassinari oliviertassinari merged commit 83882fc into mui:next Apr 21, 2019
@joshwooding joshwooding deleted the selectinput-function-component branch April 21, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select 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.

None yet

4 participants