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

[material-ui][Slider] Labels of the marks goes beyond the block #40544

Open
DonikaV opened this issue Jan 11, 2024 · 7 comments · Fixed by #40647
Open

[material-ui][Slider] Labels of the marks goes beyond the block #40544

DonikaV opened this issue Jan 11, 2024 · 7 comments · Fixed by #40647
Assignees
Labels
component: slider This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material

Comments

@DonikaV
Copy link
Contributor

DonikaV commented Jan 11, 2024

Steps to reproduce

Link to live example: (required)

Steps:

  1. Put Slider component to the page
  2. Add marks min and max
  3. Marks goes beyond the block

Current behavior

No response

Expected behavior

Marks fit the parent container

Context

image As you can see they are not in the container

Your environment

npx @mui/envinfo
  System:
    OS: macOS 14.1.2
  Binaries:
    Node: 20.9.0 - /usr/local/bin/node
    npm: 10.1.0 - /usr/local/bin/npm
    pnpm: Not Found
  Browsers:
    Chrome: 120.0.6099.216
    Edge: Not Found
    Safari: 17.1.2
  npmPackages:
    @emotion/react: ^11.11.3 => 11.11.3 
    @emotion/styled: ^11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.31 
    @mui/core-downloads-tracker:  5.15.4 
    @mui/icons-material: ^5.15.4 => 5.15.4 
    @mui/material: ^5.15.4 => 5.15.4 
    @mui/material-nextjs: ^5.15.3 => 5.15.4 
    @mui/private-theming:  5.15.4 
    @mui/styled-engine:  5.15.4 
    @mui/system:  5.15.4 
    @mui/types:  7.2.13 
    @mui/utils:  5.15.4 
    @mui/x-date-pickers: ^6.18.7 => 6.18.7 
    @types/react: ^18.2.47 => 18.2.47 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    typescript: ^5.3.3 => 5.3.3 
</details>


**Search keywords**: slider
@DonikaV DonikaV added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 11, 2024
@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Jan 11, 2024
@zanivan zanivan changed the title Slider labels goes beyond the block. [material-ui][Slider] Labels of the marks goes beyond the block Jan 11, 2024
@zanivan
Copy link
Contributor

zanivan commented Jan 11, 2024

Hey @DonikaV, thanks for the feedback!

Just to clarify, by making the marks fit the parent container

You mean:

1. Make a way to align the labels to the edges of the slider's track

Screenshot 2024-01-11 at 17 04 58

or

2. Make the parent container encompass the labels?

Screenshot 2024-01-11 at 17 14 05

@zanivan zanivan added package: material-ui Specific to @mui/material and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 11, 2024
@KumarNitin19
Copy link
Contributor

Hey @zanivan, @mj12albert I think the second option which is encompassing the labels by parent container would be a better solution. If you also think that it is better than please assign this to me i will fix this issue.

@DonikaV
Copy link
Contributor Author

DonikaV commented Jan 12, 2024

Hey @DonikaV, thanks for the feedback!

Just to clarify, by making the marks fit the parent container

You mean:

1. Make a way to align the labels to the edges of the slider's track

Screenshot 2024-01-11 at 17 04 58

or

2. Make the parent container encompass the labels?

Screenshot 2024-01-11 at 17 14 05

The first option will be great, as a workaround I removed labels from the marks and added my custom HTML for labels. But will be great to have something like in your screenshot #1

@zanivan
Copy link
Contributor

zanivan commented Jan 12, 2024

If we go with the first case, I believe we should make it in a way where the user need to opt in to this feature—that mean that I wouldn't change the current default.

On the second case, though, maybe it could be a breaking change. I wonder if changing the component to encompass the labels like this wouldn't break the existing layouts.

A third way, would be to add @DonikaV's workaround—or another one—as a demo to the docs, so users would be able to learn how to do so.


Any thoughts @DiegoAndai @mj12albert ?

@DonikaV
Copy link
Contributor Author

DonikaV commented Jan 13, 2024

This is what i did.

<Grid item xs={12} className="pt-0">
                    <div className="text-center">
                      <Slider
                        marks={marks}
                        step={10}
                        defaultValue={MIN}
                        valueLabelDisplay="auto"
                        min={MIN}
                        max={MAX}
                        onChange={handleChange}
                      />
                    </div>
                    <div className="slider-marks d-flex flex-between">
                      <div role="button" onClick={() => setVal(MIN)}>
                        {formatCurrency(MIN)} min
                      </div>
                      <div role="button" onClick={() => setVal(MAX)}>
                        {formatCurrency(MAX)} max
                      </div>
                    </div>
                  </Grid>
              Along with the styles:
               ```.d-flex {
                  display: flex;
                 }
               .flex-between {
                    justify-content: space-between;
                    align-items: center;
               } ```

Result:
image

P.S. I am ok with both options, the most important is that they will be visible, and inside the parent container. As I know in MUI flutter is the same issue, my colleague also added custom min and max labels.

@DiegoAndai
Copy link
Member

A third way, would be to add @DonikaV's workaround—or another one—as a demo to the docs, so users would be able to learn how to do so.

I'm inclined to this one:

  • Add a demo for it now
  • Revisit this for Material 3

@DonikaV would you be able to open a PR to add the example to the docs?

@DiegoAndai DiegoAndai added this to the Material UI: v7 draft milestone Jan 16, 2024
@DiegoAndai DiegoAndai assigned DiegoAndai and unassigned mj12albert Jan 16, 2024
DonikaV added a commit to DonikaV/material-ui that referenced this issue Jan 17, 2024
Added example how to add custom labels to the slider because of this issue
 mui#40544
@DonikaV
Copy link
Contributor Author

DonikaV commented Jan 17, 2024

A third way, would be to add @DonikaV's workaround—or another one—as a demo to the docs, so users would be able to learn how to do so.

I'm inclined to this one:

  • Add a demo for it now
  • Revisit this for Material 3

@DonikaV would you be able to open a PR to add the example to the docs?

Hello, done, #40647 Please check if all is good. This is my first contribution to such big project :)

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! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants