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] Change Slider.Output to Slider.Value and add format prop #990

Merged
merged 12 commits into from
Dec 12, 2024

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Dec 9, 2024

Part of #961

@mj12albert mj12albert added the component: slider This is the name of the generic UI component, not the React module! label Dec 9, 2024
@mui-bot
Copy link

mui-bot commented Dec 9, 2024

Netlify deploy preview

https://deploy-preview-990--base-ui.netlify.app/

Generated by 🚫 dangerJS against a2fe73e

@mj12albert mj12albert force-pushed the feat/format-slider branch 2 times, most recently from f04ddaa to 2f57fe2 Compare December 9, 2024 10:09
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 9, 2024
@colmtuite colmtuite requested a review from vladmoroz December 9, 2024 20:11
@colmtuite colmtuite added this to the 1.0.0-alpha.1 release milestone Dec 9, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 10, 2024
@mj12albert mj12albert marked this pull request as ready for review December 10, 2024 03:18
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Dec 10, 2024
@mj12albert mj12albert force-pushed the feat/format-slider branch 2 times, most recently from cae92e2 to 72dba3a Compare December 11, 2024 12:49
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 11, 2024
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Can you please update the demo on the new docs page and ask Vlad for a review? The implementation itself looks good, but the comment I added above causes issues on the docs.

Screenshot 2024-12-11 at 19 30 16

Comment on lines 11 to 17
* Demos:
*
* * [Slider](https://base-ui.com/components/react-slider/)
*
* API:
*
* * [SliderValue API](https://base-ui.com/components/react-slider/#api-reference-SliderValue)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be here, it's using the old format.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 12, 2024
@@ -1,6 +1,9 @@
{
Copy link
Member Author

@mj12albert mj12albert Dec 12, 2024

Choose a reason for hiding this comment

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

@vladmoroz The *.Value components also support children as a function (with some props) for formatting: https://github.com/mui/base-ui/blob/master/docs/data/components/select/select.mdx?plain=1#L179-L183

Should we add them to docs/reference/overrides to show them in the props table as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add them to docs/reference/overrides to show them in the props table as well?

Yes please!

@mj12albert mj12albert merged commit 4087bb1 into mui:master Dec 12, 2024
23 checks passed
@mj12albert mj12albert deleted the feat/format-slider branch December 12, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants