Skip to content

Conversation

@bernhard-adobe
Copy link
Contributor

@bernhard-adobe bernhard-adobe commented Nov 16, 2022

Core tokens migration for Slider.
Includes t-shirt sizing

Description

Migrated and modded all vars.
Added t-shirt sizing

ticket: https://jira.corp.adobe.com/projects/CSS/issues/CSS-177

Issues Addressed

How and where has this been tested?

Screenshots

Screen Shot 2022-11-15 at 22 36 00

Screen Shot 2022-11-15 at 22 36 02

Firefox:
Screen Shot 2022-11-15 at 22 39 31

Safari:
Screen Shot 2022-11-15 at 22 39 33

QA notes

  • Validate circle alignment on tick marks
  • Validate circle alignment at edges

To-do list

  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have tested these changes in Windows High Contrast mode.
  • This pull request is ready to merge.

@bernhard-adobe bernhard-adobe force-pushed the bschmidt/CSS-177-slider-core-tokens branch from cd669f3 to 53fc5e3 Compare November 17, 2022 23:21
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

🚀 Deployed on https://pr-1547--spectrum-css.netlify.app

@github-actions github-actions bot temporarily deployed to pull request November 17, 2022 23:25 Inactive
Copy link
Contributor

@lunarfusion lunarfusion left a comment

Choose a reason for hiding this comment

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

Ignore anything that isn't helpful, of course!

&:last-of-type {
&:before {
border-start-start-radius: 0;
border-end-start-radius: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think lines 176 and 177 are redundant of lines 158 and 159.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they all are! unclear why they were added in the non-core-tokens version.

@github-actions github-actions bot temporarily deployed to pull request November 28, 2022 17:38 Inactive
@github-actions github-actions bot temporarily deployed to pull request November 29, 2022 04:22 Inactive
@bernhard-adobe
Copy link
Contributor Author

@lunarfusion I addressed your feedback! Thanks a lot for the impressive review, excellent work!

The only thing missing here is the top centering that I can't get to work as mentioned above. The handle aligns in the center of the entire Slider height, not just the center of the track.
Did you try this out in the browser or did you check out the branch locally and pasted in the code?

Thanks for pointing me to a good solution!

@github-actions github-actions bot temporarily deployed to pull request November 30, 2022 01:12 Inactive
Copy link
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

It looks like there might be a mismatch between the track color and the tick color (zoomed in to see the difference) - or - maybe there's a z-index issue here?

Screen Shot 2022-12-01 at 4 13 56 PM

Also, with regard to the tick position & the handle, there's an open PR from last year that you might be able to get some info from.

@pfulton pfulton requested a review from castastrophe December 2, 2022 15:48
@bernhard-adobe
Copy link
Contributor Author

bernhard-adobe commented Dec 6, 2022

It looks like there might be a mismatch between the track color and the tick color (zoomed in to see the difference) - or - maybe there's a z-index issue here?

Screen Shot 2022-12-01 at 4 13 56 PM

Also, with regard to the tick position & the handle, there's an open PR from last year that you might be able to get some info from.

Nah, it's just the wrong color. we don't use tokens for the ticks and even in the current version of slider there is a color bug for the disabled ticks.
Screen Shot 2022-12-05 at 20 26 26
https://opensource.adobe.com/spectrum-css/slider.html#tickwithlabels

I have fixed the wrong color. good catch.
Screen Shot 2022-12-05 at 19 51 21
Screen Shot 2022-12-05 at 19 51 21

@github-actions github-actions bot temporarily deployed to pull request December 6, 2022 04:28 Inactive
Copy link
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

Thanks for fixing up the positioning of the tick and the handle!

I noticed a few new things in this most recent review:

It looks like the spacing on the right side of the handle has been adversely affected by the recent changes:

Screen Shot 2022-12-06 at 10 15 54 AM

It's particularly noticeable on the slider with two handles:
Screen Shot 2022-12-06 at 10 16 05 AM

Also, the tick/track color difference seems to still be an issue:
Screen Shot 2022-12-06 at 10 21 06 AM

Finally, would you mind double-checking on the Windows High Contrast Mode styles? It looks like (at least) the standard, non-disabled version of the track color needs to be fixed:

Your branch:
Screen Shot 2022-12-06 at 10 22 34 AM

Current production:
Screen Shot 2022-12-06 at 10 23 04 AM

@github-actions github-actions bot temporarily deployed to pull request December 7, 2022 08:08 Inactive
Copy link
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

We're almost there on the Windows High Contrast Mode styles! Two things I noticed: the ticks are missing, and the ramp seems to be different now vs. what's live in production.

Your branch:
Screen Shot 2022-12-08 at 11 10 28 AM

Production:
Screen Shot 2022-12-08 at 11 10 16 AM

Could you fix up those examples?

@bernhard-adobe
Copy link
Contributor Author

bernhard-adobe commented Dec 8, 2022

I'm not sure that this suggestion actually got implemented (or, at least it doesn't seem to be in the codebase, so maybe it didn't get committed?) because I still see the misalignment. Mind taking a quick look?

@pfulton this actually adds a wider space between the slider handle on the right side between the handle and the track. I would suggest to maybe not implement this solution.
Screen Shot 2022-12-08 at 12 56 25

Screen Shot 2022-12-08 at 12 56 41

@pfulton
Copy link
Collaborator

pfulton commented Dec 8, 2022

@pfulton this actually adds a wider space between the slider handle on the right side between the handle and the track. I would suggest to maybe not implement this solution.

Ok. Because this has been a long-standing issue with this component in our docs site, we still need to fix this problem before we merge the PR in. If you get stuck on finding a solution, please mention it in the spectrum_css_dev channel so that more folks can lend a hand.

@castastrophe
Copy link
Contributor

In case they haven't been highlighted yet, I wanted to make sure we'd addressed the following PRs here since their updates won't apply once this merges: #1005, #1133, #1206, #1289

@bernhard-adobe
Copy link
Contributor Author

Thanks @castastrophe .
We got the following PRs covered in https://jira.corp.adobe.com/browse/CSS-332 Clean up old Slider PRs:
#1206 and #1289

I will add to the ticket the PRs and ping @pfulton about the updates.
#1005, #1133

@mlogsdon18 mlogsdon18 force-pushed the bschmidt/CSS-177-slider-core-tokens branch from 6c7f2da to e1460a5 Compare March 29, 2023 15:53
@mlogsdon18 mlogsdon18 self-assigned this Mar 29, 2023
@mlogsdon18 mlogsdon18 closed this Mar 29, 2023
@mlogsdon18 mlogsdon18 reopened this Mar 29, 2023
@mlogsdon18 mlogsdon18 closed this Mar 29, 2023
@github-actions github-actions bot temporarily deployed to pull request March 29, 2023 17:04 Inactive
@castastrophe castastrophe deleted the bschmidt/CSS-177-slider-core-tokens branch May 26, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[slider] The slider controls margin makes it harder to implement click logic correctly.

6 participants