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

[Slider] Fix textAlign prop affecting Slider rail #16440

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

mohan-cao
Copy link
Contributor

@mohan-cao mohan-cao commented Jul 1, 2019

This change affects styling for the Slider component, and should fix #16437
I have looked over the tests but see that #16416 is still unmerged.
My first PR here, so feedback very welcome. :)

Closes #16437

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 1, 2019

Details of bundle changes.

Comparing: 7fb9b40...9b37ad2

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 320,029 320,029 88,270 88,270
@material-ui/core/Paper 0.00% 0.00% 68,291 68,291 20,369 20,369
@material-ui/core/Paper.esm 0.00% 0.00% 61,573 61,573 19,161 19,161
@material-ui/core/Popper 0.00% 0.00% 28,945 28,945 10,395 10,395
@material-ui/core/Textarea 0.00% 0.00% 5,507 5,507 2,358 2,358
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,578 1,578
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,009 16,009 5,791 5,791
@material-ui/core/useMediaQuery 0.00% 0.00% 2,597 2,597 1,102 1,102
@material-ui/lab +0.02% 🔺 0.00% 140,210 140,242 43,456 43,458
@material-ui/styles 0.00% 0.00% 51,698 51,698 15,348 15,348
@material-ui/system 0.00% 0.00% 15,420 15,420 4,391 4,391
Button 0.00% 0.00% 84,316 84,316 25,722 25,722
Modal 0.00% 0.00% 14,427 14,427 5,087 5,087
Portal 0.00% 0.00% 3,473 3,473 1,572 1,572
Slider +0.04% 🔺 +0.02% 🔺 74,889 74,921 23,293 23,297
colorManipulator 0.00% 0.00% 3,904 3,904 1,544 1,544
docs.landing 0.00% 0.00% 54,833 54,833 13,892 13,892
docs.main 0.00% 0.00% 647,269 647,269 203,922 203,922
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 293,180 293,180 84,176 84,176

Generated by 🚫 dangerJS against 9b37ad2

@mbrookes mbrookes added component: slider This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Jul 1, 2019
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.

Thanks for working on it. Unfortunately the suggested change breaks the vertical slider.

@oliviertassinari
Copy link
Member

Interesting, it's most likely linked to the issue reported by @leoyli #16416 (comment). Changing the text align reproduce a similar issue with the vertical slider.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 1, 2019

It seems that changing the display from inline to block fixes the two occurrences of the issue:

Capture d’écran 2019-07-01 à 18 55 00
Capture d’écran 2019-07-01 à 18 55 06

@mohan-cao
Copy link
Contributor Author

Sweet, thanks, that works.

@oliviertassinari
Copy link
Member

@mohan-cao It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@mohan-cao mohan-cao deleted the slider-fix branch July 2, 2019 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: slider 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.

Material UI slider rail incorrectly aligned with alternate textAlign property
4 participants