fix(rating): address inconsistent hover behavior #3196

cdransf · 2024-10-02T19:30:42Z

Description

Provides more granular control over the hover behavior of child stars within the rating component to prevent hovering in space adjacent to the component from highlighting all stars.

CSS-735

How and where has this been tested?

Verified in local Storybook story for rating component.

Validation steps

Fetch the branch and run it locally or open the Storybook URL for the PR.
Navigate to the rating component.
Verify that hovering over rating stars is now more controlled and that hovering between stars doesn't result or all/no stars being displayed as active.
Verify that changing the selected star works as expected with the hover behavior.

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
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.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-10-02T19:30:46Z

🦋 Changeset detected

Latest commit: 82cbeeb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/rating	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-10-02T19:34:16Z

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

github-actions · 2024-10-02T19:35:32Z

File metrics

Summary

Total size: 4.30 MB^*
Total change (Δ): 🔴 ⬆ 0.61 KB (0.01%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
rating	9.99 KB	🔴 ⬆ 0.20 KB

Details

rating

Filename	Head	Compared to base
index-base.css	9.21 KB	🔴 ⬆ 0.20 KB (2.20%)
index-theme.css	1.39 KB	-
index-vars.css	9.99 KB	🔴 ⬆ 0.20 KB (2.02%)
index.css	9.99 KB	🔴 ⬆ 0.20 KB (2.02%)
themes/express.css	1.02 KB	-
themes/spectrum.css	1.01 KB	-

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

jawinn

When testing, I'm seeing a couple issues:

The focus ring should not be appearing on click. The existing is-focused is actually just for keyboard focus.

Really.is-focused should be named .is-keyboardFocused or even better the selector could be &:has(:focus-visible), &.is-keyboardFocused { ... }, though I'm not sure we want to do that sort of renaming now? If we're already doing a breaking change, we could, though I'm hesitant to increase the scope of this work.

I'd definitely reword the docs sentence that says "When the rating receives focus" to "When the rating receives keyboard focus".
Clicking on any of the ratings on the Docs page doesn't update something with the current value; the underline will show in the wrong place.
The is-hovered class gets leftover on elements when Rating is no longer being hovered

jawinn · 2024-10-04T15:46:47Z

components/rating/index.css

@@ -168,6 +157,16 @@
 			display: block;
 		}
 	}
+
+	&.is-hovered {


The requirement for this class and how it should be used needs to be included in usage notes within the documentation. Within the PR description would be helpful as well.

As I am understanding it, previously CSS would set all stars filled on hover of the whole component, and then all stars following the hovered star unfilled using the subsequent-sibling combinator (~). And that will not work when you hover on the gap between items. And with this change you are looking at having a class that the implementation will apply to the hovered star as well as all previous stars? And those classes should stay until hover or focus leaves the component?

I think that could work. Perhaps this class should be something like .is-hoverSelection? It seems odd to also have is-hovered on elements that aren't actually hovered. It might also be worth adding the hover pseudo to the selector :hover, &.is-hoverSelection.

Another option would be using padding instead of gaps, but I don't know if we want to go that route or not.

Got it! I'll add this to the PR description and will look for the usage notes.

That is an accurate description of the issue — I first approached it using CSS but there's an issue with one of our postcss plugins when trying to style one of the stars based on the hover state of the next: saulhardman/postcss-hover-media-feature#35.

I've updated the selector to use :hover, .is-hoverSelection. The latter is now removed from all child icons on mouseout of the parent component.

I've updated is-focused to be is-keyboardFocused and changed the state in the stories and templates too.

Clicking on any of the ratings on the Docs page doesn't update something with the current value; the underline will show in the wrong place.

I need to look at this a bit more — it doesn't look like the instance in the docs is making it into the @change handler that updates this (as it works in the stories for the component):

@change=${function(e) { const rating = e.target.closest(`.${rootClass}`); if (!rating) return; const input = rating.closest(`.${rootClass}-input`); if (!input) return; if (!isReadOnly && !isDisabled) { updateArgs({ value: parseInt(input.value, 10) }); } }}

.changeset/spotty-penguins-sort.md

components/rating/stories/rating.stories.js

marissahuysentruyt

I had a brainstorm while I was reviewing this- we could totally put a little disclaimer on the docs page that the rating components "have limited functionality" or "are not fully functional." Then add a "visit the default story for extended functionality" line or something (with a link to the default story page). What do you think about that? I did that with breadcrumbs actually.

marissahuysentruyt · 2024-10-16T19:17:34Z

components/rating/stories/rating.stories.js

@@ -59,7 +61,7 @@ export default {
 };

 /**
- * A initial value of three is used for the following examples, to demonstrate both active and inactive stars.
+ * An initial value of three is used for the following examples, to demonstrate both active and inactive stars.


👏 love a typo fix

marissahuysentruyt · 2024-10-17T13:45:34Z

components/rating/stories/rating.test.js

@@ -18,8 +18,8 @@ export const RatingGroup = Variants({
 			isDisabled: true,
 		},
 		{
-			testHeading: "Focused",
-			isFocused: true,
+			testHeading: "Keyboard focused",


Just personal preference here- I'd probably hyphenate this. I'm not sure we have a very strong convention for that either way though, so you can disregard if you want.

marissahuysentruyt · 2024-10-17T13:59:25Z

components/rating/index.css

-	/* All stars following the hovered star */
-	&:hover ~ .spectrum-Rating-icon {
+	&:hover,
+	&.is-hoverSelection {


Is the intention here that this new .is-hoverSelection class (which gets added in your JS) sort of replaces this sibling combinator ~? Or no- this change removes the functionality of hovering over any part of the component and all stars fill?

Why does that change reverse the display values for the active & inactive stars?

marissahuysentruyt · 2024-10-17T14:06:01Z

components/rating/stories/template.js

+			});
+
+			icon.addEventListener("mouseleave", function(event) {
+				if (!rating.contains(event.relatedTarget)) {


I'm not sure I've seen relatedTarget. Could you explain what this if is checking for? (I'll have to look it up!)

marissahuysentruyt · 2024-10-17T14:27:46Z

components/rating/stories/template.js

+				if (index <= hoverIndex ||
+					(index <= selectedIndex && hoverIndex === -1)
+				) {
+					icon.classList.add("is-hoverSelection");
+					activeStar.style.display = "block";
+					inactiveStar.style.display = "none";
+				}


Aren't hoverIndex and selectedIndex set to -1, which means that index will always be greater than them initially? It's working (don't get me wrong), but how is that working? I'm totally missing something here 🤦‍♀️

marissahuysentruyt · 2024-10-17T14:31:57Z

components/rating/index.css

-	/* When the entire component is hovered, show all solid icons */
-	&:hover {
-		.spectrum-Rating-starActive {
-			display: block;
-		}
-
-		.spectrum-Rating-starInactive {
-			display: none;
-		}
-	}


Did we need to double check with design or anybody else that we can completely remove this? I agree with the removal, but it certainly seems like it was intentional. Also because I'm nosy I'd love to get the context around that decision hahahaha

castastrophe · 2024-10-17T16:58:25Z

.changeset/spotty-penguins-sort.md

@@ -0,0 +1,8 @@
+---
+"@spectrum-css/rating": major


@pfulton I'm a bit anxious about introducing such a large bump to an S1 component with the current progress on foundations work. What do you think? Should we try to scale back the scope of this work, hold it until after foundations, or move forward as-is?

I agree on the anxiousness around S1/S2 timing. On the plus side, though, this is a component that is not currently available in SWC, so the downstream impact of a big change might not be as large as it could have been if it was already implemented there. That said: it's still a major, and we want to respect what that means for all potential consumers.

I think we should pause on this one for now until after Foundations goes in. After that, we can work through rebasing this one and getting it merged in. The scope of changes are only within this particular component, so hopefully any potential conflicts won't be too terrible to resolve when the time comes.

pfulton · 2024-10-22T15:25:50Z

Marking this as blocked for now until we get S2 Foundations merged into main. Once we do, we'll come back to this, get it rebased against the new main stuff, and then look to get it merged in.

…ng icons

Co-authored-by: Marissa Huysentruyt <[email protected]>

cdransf marked this pull request as ready for review October 2, 2024 19:30

cdransf changed the title ~~Cdransf/rating behavior fix~~ fix(rating): address inconsistent hover behavior Oct 2, 2024

cdransf requested review from castastrophe, pfulton and jawinn October 2, 2024 20:01

cdransf self-assigned this Oct 2, 2024

cdransf added ready-for-review skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Oct 2, 2024

cdransf force-pushed the cdransf/rating-behavior-fix branch 2 times, most recently from 3a4f7ca to 878ed6c Compare October 3, 2024 17:41

castastrophe requested a review from 5t3ph October 4, 2024 14:53

jawinn requested changes Oct 4, 2024

View reviewed changes

cdransf force-pushed the cdransf/rating-behavior-fix branch 4 times, most recently from 8470a26 to 610396a Compare October 7, 2024 15:21

adobe deleted a comment from github-actions bot Oct 7, 2024

cdransf force-pushed the cdransf/rating-behavior-fix branch 2 times, most recently from 6147f91 to f0255e7 Compare October 7, 2024 18:42

cdransf requested a review from jawinn October 7, 2024 21:51

cdransf force-pushed the cdransf/rating-behavior-fix branch 4 times, most recently from 01f2393 to 634d93d Compare October 8, 2024 18:21

marissahuysentruyt reviewed Oct 9, 2024

View reviewed changes

components/rating/stories/rating.stories.js Outdated Show resolved Hide resolved

marissahuysentruyt reviewed Oct 9, 2024

View reviewed changes

components/rating/stories/rating.stories.js Outdated Show resolved Hide resolved

marissahuysentruyt reviewed Oct 9, 2024

View reviewed changes

components/rating/stories/rating.stories.js Outdated Show resolved Hide resolved

cdransf force-pushed the cdransf/rating-behavior-fix branch 6 times, most recently from 9ea3c8e to ff42950 Compare October 16, 2024 20:53

marissahuysentruyt reviewed Oct 17, 2024

View reviewed changes

castastrophe reviewed Oct 17, 2024

View reviewed changes

cdransf force-pushed the cdransf/rating-behavior-fix branch from ff42950 to bea4f25 Compare October 17, 2024 22:28

cdransf added the size-5 L ~30-42hrs; lots of effort or complexity, most of a sprint needed to complete. label Oct 17, 2024

cdransf force-pushed the cdransf/rating-behavior-fix branch 5 times, most recently from 10a21fa to f3e2a45 Compare October 21, 2024 23:40

pfulton added the blocked See description and comments for what is blocking this issue label Oct 22, 2024

pfulton removed the ready-for-review label Oct 22, 2024

cdransf force-pushed the cdransf/rating-behavior-fix branch from f3e2a45 to 4b70482 Compare October 22, 2024 16:41

cdransf and others added 10 commits October 23, 2024 08:44

fix(rating): use indices to better track selected/hover state of rati…

b03e682

…ng icons

fix: conflict

67c21d5

fix(rating): hover refinements

90ede83

fix: conflict

ab19f19

Update components/rating/stories/rating.stories.js

45a2205

Co-authored-by: Marissa Huysentruyt <[email protected]>

Update components/rating/stories/rating.stories.js

cda6c2f

Co-authored-by: Marissa Huysentruyt <[email protected]>

fix(rating): heading order for rating component story

78ff74b

chore: resolve conflict

026b61d

fix(rating): click should not trigger keyboard focus

61555c3

chore(rating): remove stylelint disable

82cbeeb

cdransf force-pushed the cdransf/rating-behavior-fix branch from 4b70482 to 82cbeeb Compare October 23, 2024 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rating): address inconsistent hover behavior #3196

fix(rating): address inconsistent hover behavior #3196

cdransf commented Oct 2, 2024 •

edited by jawinn

Loading

changeset-bot bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

rating

jawinn left a comment •

edited

Loading

jawinn Oct 4, 2024

cdransf Oct 4, 2024

cdransf Oct 4, 2024

marissahuysentruyt left a comment

marissahuysentruyt Oct 16, 2024

marissahuysentruyt Oct 17, 2024

marissahuysentruyt Oct 17, 2024

marissahuysentruyt Oct 17, 2024

marissahuysentruyt Oct 17, 2024

marissahuysentruyt Oct 17, 2024

castastrophe Oct 17, 2024

pfulton Oct 22, 2024

pfulton commented Oct 22, 2024

fix(rating): address inconsistent hover behavior #3196

Are you sure you want to change the base?

fix(rating): address inconsistent hover behavior #3196

Conversation

cdransf commented Oct 2, 2024 • edited by jawinn Loading

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented Oct 2, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 2, 2024 • edited Loading

File metrics

Summary

rating

jawinn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marissahuysentruyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfulton commented Oct 22, 2024

cdransf commented Oct 2, 2024 •

edited by jawinn

Loading

changeset-bot bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

jawinn left a comment •

edited

Loading