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

feat(Slider): make thumb position clearer when the value is set near edge #32051

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Thulof
Copy link

@Thulof Thulof commented Jul 18, 2024

Previous Behavior

348393555-5a55ea88-a2e8-41d6-afcd-d2a957b9ec72

The current issue is, given the slider value range is 0 to 100, when the value is set to 10 or 90, the thumb is positioned very close to the edge of the track, which could look like 0 or 100 to users.

New Behavior

untitled-2024-07-19-0252

With an offset, the Slider thumb is in the proper position.

截屏2024-07-19 03 15 44

Related Issue(s)

#31985

@Thulof Thulof requested review from micahgodbolt and a team as code owners July 18, 2024 18:49
@Thulof
Copy link
Author

Thulof commented Jul 18, 2024

@microsoft-github-policy-service agree

@Thulof Thulof changed the title feat(Slider): make thumb position clearer when the value is set near … feat(Slider): make thumb position clearer when the value is set near edge Jul 18, 2024
@Thulof Thulof force-pushed the master branch 2 times, most recently from 4640038 to 8f0f5af Compare July 19, 2024 02:45
@Thulof
Copy link
Author

Thulof commented Jul 19, 2024

Hi, @micahgodbolt could you please help review this Pull Request? Thank you

@dmytrokirpa
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 8, 2024

📊 Bundle size report

✅ No changes found

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 8, 2024

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme virtual-rerender 33 40 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 627 641 5000
Button mount 301 308 5000
Field mount 1149 1101 5000
FluentProvider mount 708 697 5000
FluentProviderWithTheme mount 86 90 10
FluentProviderWithTheme virtual-rerender 33 40 10 Possible regression
FluentProviderWithTheme virtual-rerender-with-unmount 87 89 10
MakeStyles mount 873 883 50000
Persona mount 1791 1750 5000
SpinButton mount 1356 1437 5000
SwatchPicker mount 1652 1657 5000

@dmytrokirpa
Copy link
Contributor

Hi @Thulof, thank you for your contribution! There are a few errors in the CI:

  • Could you please address the formatting issues (prettier) in the packages/react-components/react-slider/library/src/components/Slider/useSliderState.tsx file?
  • Ensure to run the build locally for the react-slider package since it updates the react-slider.api.md. If there are any changes, please commit them as well.

Thank you!

@@ -0,0 +1,7 @@
{
Copy link
Collaborator

@fabricteam fabricteam Aug 8, 2024

Choose a reason for hiding this comment

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

🕵🏾‍♀️ visual regressions to review in the fluentuiv9 Visual Regression Report

Avatar Converged 2 screenshots
Image Name Diff(in Pixels) Image Type
Avatar Converged.Badge Mask RTL.chromium.png 3 Changed
Avatar Converged.badgeMask.chromium.png 5 Changed
Drawer 2 screenshots
Image Name Diff(in Pixels) Image Type
Drawer.Full Overlay Dark Mode.chromium.png 991 Changed
Drawer.overlay drawer full.chromium.png 3302 Changed
Field 1 screenshots
Image Name Diff(in Pixels) Image Type
Field.Slider.chromium.png 279 Changed
Slider Converged 18 screenshots
Image Name Diff(in Pixels) Image Type
Slider Converged.Disabled Dark Mode.chromium.png 34 Changed
Slider Converged.Horizontal - 100%.chromium.png 139 Changed
Slider Converged.Disabled High Contrast.chromium.png 122 Changed
Slider Converged.Horizontal 0 RTL.chromium.png 139 Changed
Slider Converged.Disabled.chromium.png 44 Changed
Slider Converged.Disabled Vertical.chromium.png 44 Changed
Slider Converged.Root High Contrast.chromium.png 155 Changed
Slider Converged.Horizontal 100 RTL.chromium.png 139 Changed
Slider Converged.Root Dark Mode.chromium.png 64 Changed
Slider Converged.Horizontal - 0%.chromium.png 139 Changed
Slider Converged.Vertical - 0%.chromium.png 140 Changed
Slider Converged.Root.chromium.png 92 Changed
Slider Converged.Root RTL.chromium.png 92 Changed
Slider Converged.Vertical - 100%.chromium.png 140 Changed
Slider Converged.Vertical 0 RTL.chromium.png 140 Changed
Slider Converged.Vertical RTL.chromium.png 92 Changed
Slider Converged.Vertical 100 RTL.chromium.png 140 Changed
Slider Converged.Vertical.chromium.png 92 Changed

@Thulof
Copy link
Author

Thulof commented Aug 8, 2024

Hi @Thulof, thank you for your contribution! There are a few errors in the CI:

  • Could you please address the formatting issues (prettier) in the packages/react-components/react-slider/library/src/components/Slider/useSliderState.tsx file?
  • Ensure to run the build locally for the react-slider package since it updates the react-slider.api.md. If there are any changes, please commit them as well.

Thank you!

OK

@dmytrokirpa
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@dmytrokirpa
Copy link
Contributor

dmytrokirpa commented Aug 9, 2024

  • The solution proposed in the pull request is not effective for sliders with steps:
Screen.Recording.2024-08-09.at.15.02.39.mov
image

@Thulof, please let me know if you are interested in addressing this issue, or I will proceed with it.

Thank you!

@Thulof
Copy link
Author

Thulof commented Aug 12, 2024

  • The solution proposed in the pull request is not effective for sliders with steps:

Screen.Recording.2024-08-09.at.15.02.39.mov

image @Thulof, please let me know if you are interested in addressing this issue, or I will proceed with it.

Thank you!

Yes, I will be working on it.

@Thulof
Copy link
Author

Thulof commented Aug 26, 2024

Hi @Thulof, thank you for your contribution! There are a few errors in the CI:

  • Could you please address the formatting issues (prettier) in the packages/react-components/react-slider/library/src/components/Slider/useSliderState.tsx file?
  • Ensure to run the build locally for the react-slider package since it updates the react-slider.api.md. If there are any changes, please commit them as well.

Thank you!

While working on these two tasks, I encountered some issues:

  1. I tried running yarn prettier packages/react-components/react-slider/library/src/components/Slider/useSliderState.tsx, but it did not display any errors.
  2. When I attempted to run yarn build within the react-slider directory, I encountered numerous type errors. It seems that my TSConfig is not properly recognizing the workspace. Can you please advise on the correct way to run the build process?
Screenshot 2024-08-27 00 15 50

@dmytrokirpa
Copy link
Contributor

Hello @Thulof, the commands you might be looking for are yarn nx lint react-slider and yarn nx build react-slider. Based on the screenshot you provided, it appears that the react-slider dependencies weren't built. Simply use the NX commands, and they will pre-build everything necessary for you.

Hi @Thulof, thank you for your contribution! There are a few errors in the CI:

  • Could you please address the formatting issues (prettier) in the packages/react-components/react-slider/library/src/components/Slider/useSliderState.tsx file?
  • Ensure to run the build locally for the react-slider package since it updates the react-slider.api.md. If there are any changes, please commit them as well.

Thank you!

While working on these two tasks, I encountered some issues:

  1. I tried running yarn prettier packages/react-components/react-slider/library/src/components/Slider/useSliderState.tsx, but it did not display any errors.
  2. When I attempted to run yarn build within the react-slider directory, I encountered numerous type errors. It seems that my TSConfig is not properly recognizing the workspace. Can you please advise on the correct way to run the build process?
Screenshot 2024-08-27 00 15 50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Slider design needs an update when the value is set to 10 or 90
3 participants