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 boolean logic to show thumb label #30761

Closed
wants to merge 6 commits into from

Conversation

NPetz
Copy link

@NPetz NPetz commented Jan 24, 2022

closes #30753

As per #30753 I changed the boolean logic to display a thumb label value on hover so that it displays even when disableSwap flag is set.

As per original requirements (#25547 (review)), I also made sure that only one thumb's value label is displayed at any time (except if valueLabelDisplay is set to on).

This last behavior isn't present when disableSwap is not set though. Should It be?

P.S. yarn lint and yarn prettier gave me conflicting formatting so sorry in advance if the formatting is wrong

@mui-bot
Copy link

mui-bot commented Jan 24, 2022

Details of bundle changes

Generated by 🚫 dangerJS against b4cfb18

@danilo-leal danilo-leal added the component: slider This is the name of the generic UI component, not the React module! label Jan 24, 2022
@NPetz
Copy link
Author

NPetz commented Feb 2, 2022

I made it so that , except when valueLabelDisplay is set to on, only one tooltip can appear at any time. Thus preventing tooltip overlapping each other.

Before this, pointer-events was disabled when disableSwap was set and no handle was active, but this was preventing the tooltip to appear on hover.

      <Stack spacing={2} width={500}>
        <Slider valueLabelDisplay="auto" min={0} max={100} />
        <Slider valueLabelDisplay="auto" min={0} max={100} disableSwap />
        <Slider valueLabelDisplay="auto" min={0} max={100} defaultValue={[20, 40]} />
        <Slider valueLabelDisplay="auto" min={0} max={100} defaultValue={[20, 40]} disableSwap />
      </Stack>

Before:

before.mov

After:

after.mov

@NPetz NPetz marked this pull request as ready for review February 2, 2022 14:55
@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

@NPetz
Copy link
Author

NPetz commented Feb 6, 2022

Now passing all tests!

@NPetz
Copy link
Author

NPetz commented Feb 22, 2022

@mnajdova

Hi sorry to bother you but since you said you could help with the issue I'm mentioning you.

Is there anything I can do to help move this forward?

Thanks in advance

@siriwatknp siriwatknp requested a review from mnajdova February 23, 2022 06:46
@siriwatknp siriwatknp added the PR: needs test The pull request can't be merged label Mar 1, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Mar 1, 2022

@NPetz The change looks great to me. Would you be able to add a test to make sure that no one breaks this functionality in the future?

@mnajdova
Copy link
Member

mnajdova commented Mar 2, 2022

@NPetz I apologize for the delay. The CI is passing so I guess the prettier & lint conflict was resolved. The last step would be adding tests for the following scenarios:

  • set the disableSwap prop to true and change if the thumb value label is shown when hovering the component

  • set valueLabelDisplay to auto and check if hovering one and then other thumb shows only one value lable

@NPetz
Copy link
Author

NPetz commented Mar 2, 2022

Thank you both!

I will go ahead and add a test then.

Thanks

@mnajdova
Copy link
Member

@NPetz it's been a while, are you still interested in continuing the effort? Looks like we are missing test for the new behavior :)

@NPetz
Copy link
Author

NPetz commented Jun 24, 2022

@NPetz it's been a while, are you still interested in continuing the effort? Looks like we are missing test for the new behavior :)

It has been a while!
Sorry I went on a long vacation and totally forgot about this.

I'll try to add the tests before the end of the month!

@michaldudak
Copy link
Member

Hi @NPetz, just a friendly reminder about this PR. Would you still like to contribute?

@ZeeshanTamboli
Copy link
Member

Since there are no updates, I am closing this PR.

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! PR: needs test The pull request can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Show value tooltip when hovering a slider thumb with disableSwap true
7 participants