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

chore(slider): slider handle alignment fix #3860

Merged
merged 11 commits into from
Dec 12, 2023
Merged

chore(slider): slider handle alignment fix #3860

merged 11 commits into from
Dec 12, 2023

Conversation

Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Dec 8, 2023

Description

Added DOM changes to bring in the handleContainer and trackContainer

Related issue(s)

Motivation and context

How has this been tested?

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

Screenshot 2023-12-08 at 6 23 33 PM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@Rajdeepc Rajdeepc requested a review from Westbrook December 8, 2023 13:10
@Rajdeepc Rajdeepc linked an issue Dec 8, 2023 that may be closed by this pull request
1 task
Copy link

github-actions bot commented Dec 8, 2023

Tachometer results

Chrome

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 476 kB 117.93ms - 119.74ms - unsure 🔍
-2% - +0%
-2.71ms - +0.40ms
branch 459 kB 118.72ms - 121.25ms unsure 🔍
-0% - +2%
-0.40ms - +2.71ms
-
Firefox

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 476 kB 223.02ms - 231.10ms - unsure 🔍
-4% - +1%
-8.62ms - +3.34ms
branch 459 kB 225.30ms - 234.10ms unsure 🔍
-1% - +4%
-3.34ms - +8.62ms
-

@@ -616,7 +616,7 @@ governing permissions and limitations under the License.
);
z-index: 0;
}
.ticks ~ .spectrum-Slider-handleContainer .handle {
.ticks ~ .handleContainer .handle {
background: var(
--mod-slider-ticks-handle-background-color,
var(--spectrum-slider-ticks-handle-background-color)
Copy link
Contributor

Choose a reason for hiding this comment

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

This value doesn't seem to be available, yet:
image
Vs the CSS implementation:
image
See the "tick" sticking through.

Do we maybe need to update @spectrum-css/tokens as part of this change? Or report missing values back to CSS?

Copy link
Contributor Author

@Rajdeepc Rajdeepc Dec 11, 2023

Choose a reason for hiding this comment

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

background color property in handleController.ts was overriding the tick css. Should be solved now

@Rajdeepc Rajdeepc requested a review from Westbrook December 11, 2023 07:32
@Rajdeepc Rajdeepc linked an issue Dec 11, 2023 that may be closed by this pull request
1 task
@Westbrook
Copy link
Contributor

Also, can you see if this addresses #3536 in a material way?

@Rajdeepc
Copy link
Contributor Author

Rajdeepc commented Dec 11, 2023

Also, can you see if this addresses #3536 in a material way?

Yes this fix will take care of #3536. I have added a comment on the ticket

Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

LGTM! Love hitting 🐦 🐦 🐦 with 🪨.

:shipit:

@Westbrook Westbrook merged commit bed73c0 into main Dec 12, 2023
47 checks passed
@Westbrook Westbrook deleted the bug/slider-ui branch December 12, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants