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

fix(slider): skip variant="filled" css when fill-start #4217

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Mar 26, 2024

Description

This PR adds a condition this.fillStart === undefined to the set variant method in order to address a specific issue. The condition ensures that the variant attribute is only set if the fillStart property is undefined, preventing unintended behavior when both variant and fillStart attributes are present simultaneously.

Changes

Added a condition this.fillStart === undefined to the set variant method.
Updated the logic to set or remove the variant attribute based on the new condition.

Related issue(s)

Motivation and context

How has this been tested?

<sp-slider
    max="20"
    variant="filled"
    fill-start
    min="0"
    value="10"
    step="1"
></sp-slider>
  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

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 changed the title fix(slider): no change in background for variant filled and fill start fix(slider): no change in fill with variant="filled" and fill-start Mar 26, 2024
@Rajdeepc Rajdeepc changed the title fix(slider): no change in fill with variant="filled" and fill-start fix(slider): skip variant="filled" when fill-start Mar 26, 2024
@Rajdeepc Rajdeepc changed the title fix(slider): skip variant="filled" when fill-start fix(slider): skip variant="filled" css when fill-start Mar 26, 2024
@Rajdeepc Rajdeepc linked an issue Mar 26, 2024 that may be closed by this pull request
1 task
@Rajdeepc Rajdeepc self-assigned this Mar 26, 2024
Copy link

github-actions bot commented Mar 26, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.97 0.97 0.98
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 228.594 kB 217.474 kB 217.222 kB 🏆
Scripts 60.498 kB 54.692 kB 54.541 kB 🏆
Stylesheet 35.503 kB 30.813 kB 30.721 kB 🏆
Document 5.881 kB 5.257 kB 5.248 kB 🏆
Third Party 126.712 kB 126.712 kB 126.712 kB

Request Count

Category Latest Main Branch
Total 43 43 43
Scripts 35 35 35
Stylesheet 5 5 5
Document 1 1 1
Third Party 2 2 2

Copy link

github-actions bot commented Mar 26, 2024

Tachometer results

Chrome

slider permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 477 kB 111.17ms - 113.64ms - unsure 🔍
-2% - +1%
-2.40ms - +0.68ms
branch 469 kB 112.35ms - 114.18ms unsure 🔍
-1% - +2%
-0.68ms - +2.40ms
-
Firefox

slider permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 477 kB 197.00ms - 203.20ms - unsure 🔍
-3% - +2%
-5.28ms - +3.88ms
branch 469 kB 197.43ms - 204.17ms unsure 🔍
-2% - +3%
-3.88ms - +5.28ms
-

@Rajdeepc Rajdeepc requested a review from Westbrook March 26, 2024 15:22
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.

Would it be useful to have a VRT that intersected with this change to ensure we do not regress on this?

@@ -101,7 +101,7 @@ export class Slider extends SizedMixin(ObserveSlotText(SliderHandle, ''), {
if (variant === this.variant) {
return;
}
if (variants.includes(variant)) {
if (variants.includes(variant) && this.fillStart === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need text in the readme as to the prioritization here.

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! Thanks for clearing this intersection up for consumers. 🙇🏼

@Westbrook Westbrook merged commit b6d389d into main Mar 27, 2024
49 checks passed
@Westbrook Westbrook deleted the bug/slider-variant-filled branch March 27, 2024 13:39
TarunAdobe pushed a commit that referenced this pull request Apr 1, 2024
* fix(slider): no change in background for variant filled and fill start

* chore(slider): updated readme for slider with fill start and variant filled

---------

Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local>
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 with variant="filled" AND fill-start fills in unexpected ways.
2 participants