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

[docs] Change the range slider example to be visible and workable as expected #30432

Closed
wants to merge 1 commit into from
Closed

[docs] Change the range slider example to be visible and workable as expected #30432

wants to merge 1 commit into from

Conversation

ArseniyX
Copy link

@ArseniyX ArseniyX commented Dec 28, 2021 •
edited
Loading

@abhinav-22-tech
Copy link
Contributor

Preview of Range slider before: https://mui.com/components/slider/#range-slider
image

Preview of Range slider after: https://deploy-preview-30432--material-ui.netlify.app/components/slider/#range-slider
image

hbjORbj
hbjORbj reviewed Dec 29, 2021
View reviewed changes
docs/src/pages/components/slider/RangeSlider.js
@@ -14,13 +14,13 @@ export default function RangeSlider() {
};

return (
<Box sx={{ width: 300 }}>
<Box sx={{ width: 300, mt: 4 }}>
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you added this? This makes the component a bit off the center.

Copy link
Author

Choose a reason for hiding this comment

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

mui-slider I cant see the labels

Copy link
Member

@hbjORbj hbjORbj Dec 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Inside codesandbox, yes, but it looks fine on docs page. This changes places the demo example on docs page off the center. We want to keep consistency between examples, so I'd leave it as it is for now.

Copy link
Author

@ArseniyX ArseniyX Dec 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Inside codesandbox, yes, but it looks fine on docs page. This changes places the demo example on docs page off the center. We want to keep consistency between examples, so I'd leave it as it is for now.

I understand your point of view, but as for me functionality is much more important than design. It may be possible to add 'mt: "10%"' or 'mt: "5%"' instead?

@hbjORbj hbjORbj changed the title change the slider to be visible and workable as expected [docs] Change the range slider example to be visible and workable as expected Dec 29, 2021
@hbjORbj
Copy link
Member

hbjORbj commented Dec 29, 2021 •
edited
Loading

@ArseniyX Hi, thanks for opening the PR. I don't clearly understand the scope of this PR though. It seems that you are trying to display "°C " together. I agree with that fix. Can you elaborate on your title ("to be visible and workable as expected")?

hbjORbj
hbjORbj reviewed Dec 29, 2021
View reviewed changes
docs/src/pages/components/slider/RangeSlider.js
<Slider
getAriaLabel={() => 'Temperature range'}
value={value}
onChange={handleChange}
valueLabelDisplay="auto"
getAriaValueText={valuetext}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you deleting this?

Copy link
Author

Choose a reason for hiding this comment

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

Why are you deleting this?

getAriaValueText not adds the '°C' in the end, but with 'valueLabelFormat' its work as expected

Copy link
Member

@hbjORbj hbjORbj Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

getAriaValueText and valueLabelFormat are not the same though. You can keep getAriaValueText while adding valueLabelFormat.

Copy link
Author

Choose a reason for hiding this comment

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

getAriaValueText and valueLabelFormat are not the same though. You can keep getAriaValueText while adding valueLabelFormat.

yes, perhaps you are right

@hbjORbj hbjORbj added the docs Improvements or additions to the documentation label Dec 29, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 4, 2022
@siriwatknp
Copy link
Member

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/material/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@mnajdova
Copy link
Member

mnajdova commented Feb 22, 2022 •
edited
Loading

This is not going in a direction we want to. I am closing it. Thanks for the work.

@mnajdova mnajdova closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbjORbj hbjORbj hbjORbj left review comments

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ArseniyX @abhinav-22-tech @hbjORbj @siriwatknp @mnajdova