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

[Slider] Move to core #16416

Merged
merged 4 commits into from
Jul 4, 2019
Merged

[Slider] Move to core #16416

merged 4 commits into from
Jul 4, 2019
+572 −374

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 28, 2019 •
edited
Loading

This change is breaking for people importing the component from the lab.

-import { Slider } from '@material-ui/lab';
+import { Slider } from '@material-ui/core';

@oliviertassinari oliviertassinari added the component: slider This is the name of the generic UI component, not the React module! label Jun 28, 2019
@oliviertassinari oliviertassinari requested review from eps1lon, mbrookes, pelotom and joshwooding and removed request for eps1lon, mbrookes and pelotom June 28, 2019 14:44
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 28, 2019 •
edited
Loading

@material-ui/core: parsed: +3.52% , gzip: +3.65%

Details of bundle changes.

Comparing: 73e9950...605ea68

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +3.52% 🔺 +3.65% 🔺 319,998 331,247 88,297 91,522
@material-ui/core/Paper 0.00% -0.02% 68,292 68,293 20,371 20,367
@material-ui/core/Paper.esm 0.00% +0.01% 🔺 61,574 61,574 19,162 19,164
@material-ui/core/Popper -0.01% +0.06% 🔺 28,945 28,942 10,401 10,407
@material-ui/core/Textarea -0.04% -0.08% 5,507 5,505 2,362 2,360
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,579 1,579
@material-ui/core/styles/createMuiTheme +0.01% 🔺 +0.03% 🔺 16,010 16,011 5,790 5,792
@material-ui/core/useMediaQuery -0.08% 0.00% 2,597 2,595 1,103 1,103
@material-ui/lab -0.09% +0.06% 🔺 140,286 140,155 43,459 43,484
@material-ui/styles 0.00% -0.05% 51,699 51,699 15,346 15,339
@material-ui/system +0.05% 🔺 +0.16% 🔺 15,420 15,428 4,390 4,397
Button -0.00% +0.03% 🔺 84,443 84,442 25,723 25,732
Modal -0.01% +0.14% 🔺 14,534 14,533 5,087 5,094
Portal -0.06% +0.06% 🔺 3,473 3,471 1,573 1,574
Slider -0.15% -0.04% 75,108 74,994 23,306 23,296
colorManipulator 0.00% -0.06% 3,904 3,904 1,544 1,543
docs.landing 0.00% 0.00% 54,338 54,338 13,762 13,762
docs.main 0.00% -0.00% 646,957 646,957 203,886 203,885
packages/material-ui/build/umd/material-ui.production.min.js +3.60% 🔺 +3.66% 🔺 292,982 303,536 84,082 87,156

Generated by 🚫 dangerJS against 605ea68

@oliviertassinari oliviertassinari force-pushed the slider-to-core branch 3 times, most recently from cb08d1f to f59ef27 Compare June 28, 2019 15:10
@oliviertassinari
Copy link
Member Author

The test coverage is pretty poor. I will try to migrate to testing-library.

@oliviertassinari oliviertassinari force-pushed the slider-to-core branch 5 times, most recently from 88cbe40 to d179297 Compare June 29, 2019 00:05
@oliviertassinari oliviertassinari force-pushed the slider-to-core branch 2 times, most recently from 4171aad to 4b6da7c Compare June 30, 2019 16:34
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 30, 2019 •
edited
Loading

The test coverage of the Slider.js file is at 98.38% now, it should be good enough.

mbrookes
mbrookes approved these changes Jun 30, 2019
View reviewed changes
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

LGTM

@leoyli
Copy link

leoyli commented Jul 1, 2019 •
edited
Loading

I have found the Slider at vertical orientation is not correctly centred. Set margin-left: -5px -> margin-left: -3px would fix this. Should I file an issue or it is so easy and can be fixed here?!

Evimics_Rx

PS. set this at horizontal mode seems not get effected (and now it is done right).

eps1lon
eps1lon previously requested changes Jul 1, 2019
View reviewed changes
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.

Can't we just re-export the Slider in lab from core? I know it's lab and all but the non-breaking change is low hanging fruit IMO.

packages/material-ui/src/Slider/Slider.js Show resolved Hide resolved
packages/material-ui/src/test-utils/createMount.js Outdated Show resolved Hide resolved
packages/material-ui/src/test-utils/describeConformance.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Show resolved Hide resolved
packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
@mohan-cao mohan-cao mentioned this pull request Jul 1, 2019
Merged
1 task
@oliviertassinari
Copy link
Member Author

@leoyli Check #16440, I believe that your issue will be taken care of. (Unless you have a reproduction?)

Can't we just re-export the Slider in lab from core? I know it's lab and all but the non-breaking change is low hanging fruit IMO.

@eps1lon Sure, It's a great idea. I'm adding a re-export as well as a warning. We can kill it in a couple of months.

@oliviertassinari
Copy link
Member Author

Oops.

@oliviertassinari oliviertassinari force-pushed the slider-to-core branch 2 times, most recently from 311e8d6 to 3228eec Compare July 3, 2019 15:45
@oliviertassinari oliviertassinari self-assigned this Jul 3, 2019
@oliviertassinari oliviertassinari force-pushed the slider-to-core branch 2 times, most recently from cd497a8 to 187b3d6 Compare July 3, 2019 20:54
@oliviertassinari oliviertassinari force-pushed the slider-to-core branch 2 times, most recently from 5c0a3a8 to f503efe Compare July 3, 2019 21:27
@oliviertassinari oliviertassinari removed their assignment Jul 3, 2019
eps1lon
eps1lon approved these changes Jul 4, 2019
View reviewed changes
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.

Very nice.

@oliviertassinari oliviertassinari merged commit dc981b6 into mui:master Jul 4, 2019
@oliviertassinari oliviertassinari deleted the slider-to-core branch July 4, 2019 10:42
@oliviertassinari
Copy link
Member Author

@eps1lon Thanks for the reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon approved these changes

@mbrookes mbrookes mbrookes approved these changes

@joshwooding joshwooding Awaiting requested review from joshwooding

Assignees
No one assigned
Labels
component: slider This is the name of the generic UI component, not the React module! test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oliviertassinari @mui-pr-bot @leoyli @mbrookes @eps1lon