Skip to content

fix(slider): slider handle aligns with track when font size changes #5372

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Feb 16, 2023
Merged

fix(slider): slider handle aligns with track when font size changes #5372

merged 18 commits into from
Feb 16, 2023

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Sep 21, 2022 •
edited
Loading

Related Issue: #4721

Summary:

This PR will align slider thumb handles with track with respect to font-size of the browser.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Sep 21, 2022
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Sep 29, 2022
@anveshmekala anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Sep 29, 2022
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 14, 2022
@anveshmekala anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Feb 8, 2023
@anveshmekala anveshmekala marked this pull request as ready for review February 10, 2023 17:49
@anveshmekala anveshmekala requested a review from a team as a code owner February 10, 2023 17:49
@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 10, 2023
@anveshmekala anveshmekala marked this pull request as draft February 10, 2023 17:49
@anveshmekala anveshmekala marked this pull request as ready for review February 10, 2023 18:08
@anveshmekala anveshmekala marked this pull request as draft February 10, 2023 18:08
anveshmekala
anveshmekala commented Feb 13, 2023
View reviewed changes
src/components/slider/slider.stories.ts Outdated
</head>
<body>
<calcite-slider label-handles min-value="50" max-value="70" range></calcite-slider>
<calcite-slider label-handles value="70"></calcite-slider>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we add all types of slider's available to users in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up adding more slider variants in a single test. This can be hard to maintain in terms of updating the test but adds value in identifying regression for larger font-size.

@anveshmekala anveshmekala marked this pull request as ready for review February 13, 2023 16:27
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 13, 2023
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 14, 2023
jcfranco
jcfranco reviewed Feb 14, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Is there a way we could achieve this primarily with HTML + CSS?

@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 15, 2023
@anveshmekala
Copy link
Contributor Author

anveshmekala commented Feb 15, 2023 •
edited
Loading

Is there a way we could achieve this primarily with HTML + CSS?

I am able to prototype the solution using CSS . Earlier i assumed the handle element doesn't scale with increase in font-size (track, ticks & font of labels are scaling wrt font-size) which is why we need to determine the translate co-ordinates for thumb dynamically.
The main reason for blocking auto scaling with browser is due the use of px sizing rather than rem me thinks. Replaced the existing px value with rem for CSS variables in slider which can make the thumb handles bigger when the font-size in browser settings increase .

@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 15, 2023
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 15, 2023
jcfranco
jcfranco approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Awesome!

@anveshmekala anveshmekala merged commit 780df6c into master Feb 16, 2023
@anveshmekala anveshmekala deleted the anveshmekala/4721-fix-slider-handle-alignment branch February 16, 2023 22:34
benelan added a commit that referenced this pull request Feb 20, 2023
@benelan
Merge branch 'benelan/docker' of github.com:esri/calcite-components i…
b8fcc60 
…nto benelan/docker

* 'benelan/docker' of github.com:esri/calcite-components:
  chore(handle): add message bundles for translation. (#6493)
  chore(release): 1.0.8-next.0
  chore(linting): block commits if there are unused imports (#6487)
  test(modal): restore fixed E2E tests (#6486)
  fix(slider): slider handle aligns with track when font size changes (#5372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

Assignees

@anveshmekala anveshmekala

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2023 February Priorities Patch (1.0.8...
Development

Successfully merging this pull request may close these issues.
3 participants
@anveshmekala @jcfranco @geospatialem