-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[charts] Update zoom slider design #17682
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
Merged
bernardobelchior
merged 7 commits into
mui:master
from
bernardobelchior:update-zoom-slider-design
May 6, 2025
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b21966a
[charts] Update zoom slider design
bernardobelchior 06a76c3
Gen docs. Remove size mention in docs.
bernardobelchior 858fac3
Add margin
bernardobelchior 2d494a1
Update demo
bernardobelchior e6ba3e3
Update color
bernardobelchior df62fa7
Update demo
bernardobelchior 47aab94
Avoid exporting internal constants
bernardobelchior File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you removed this option because the size might vary according to the presence or not of the preview.
If yes, I would be in favor of keeping it, and adapting the default value according to the presence (or not) of a preview (when we introduce it).
This size does not need to be the size of the slider itself, but the size allocated to render the slider.
Basically here you would keep all you constants, and use the
ZOOM_SLIDER_SIZE
as a default value forzoom.slider.size
. Such that users could create smaller/larger sliders and use adapt this valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this option because the size is now kind of fixed. If we increase the size, how do we scale all the components of the slider?
And center the slider in that size? Yeah, I could do that. We would need to have a minimum value, though, to avoid bleeding onto other elements of the chart. We could remove the internal concept of margin in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me you can keep all the rest as it is.
The component renders a fixed size slider, but the size available for the slider can be customized.
For now the only usage would be if a user create his own super small slider and then need to set
zoom.slider.size = 10
.But I assume with time we will make the creation of custom slider easier and easier so it worths keeping this entry point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll bring that code back in a follow-up where we allow the slider to be customized using a slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR: #17736