Skip to content

fix(rating): address inconsistent hover behavior #3196

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

Closed
cdransf wants to merge 11 commits into from
Closed

fix(rating): address inconsistent hover behavior #3196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
494769c
chore: resolve conflict
cdransf Dec 17, 2024
da7d7a3
fix(rating): use indices to better track selected/hover state of rati…
cdransf Sep 30, 2024
1407ea5
fix: conflict
cdransf Oct 14, 2024
0bc67fe
fix(rating): hover refinements
cdransf Oct 7, 2024
6d98fa8
fix: conflict
cdransf Oct 14, 2024
bb9e22e
Update components/rating/stories/rating.stories.js
cdransf Oct 9, 2024
7af0245
Update components/rating/stories/rating.stories.js
cdransf Oct 9, 2024
7c2d326
fix(rating): heading order for rating component story
cdransf Oct 9, 2024
08a62c5
chore: resolve conflict
cdransf Oct 23, 2024
f27e7a6
fix(rating): click should not trigger keyboard focus
cdransf Oct 14, 2024
eaf9568
chore(rating): remove stylelint disable
cdransf Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/spotty-penguins-sort.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@spectrum-css/rating": major
Copy link
Copy Markdown
Contributor

@castastrophe castastrophe Oct 17, 2024

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Collaborator

@pfulton pfulton Oct 22, 2024

Choose a reason for hiding this comment

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

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.

---

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.

- `.is-focused` has been renamed to `.is-keyboardFocused` to better reflect its intended use. Clicking the rating component no longer renders the focus ring.
- `.is-hoverSelection` has been added to the rating component and may be leveraged to update the hover and select state of the star icons the component contains.
24 changes: 7 additions & 17 deletions components/rating/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
}

.spectrum-Rating {
&.is-focused {
Comment thread
marissahuysentruyt marked this conversation as resolved.
&:has(:focus-visible),
&.is-keyboardFocused {
box-shadow: 0 0 0 var(--mod-rating-focus-indicator-thickness, var(--spectrum-rating-focus-indicator-thickness)) var(--highcontrast-rating-focus-indicator-color, var(--mod-rating-focus-indicator-color, var(--spectrum-rating-focus-indicator-color)));

.spectrum-Rating-icon {
Expand Down Expand Up @@ -98,17 +99,6 @@
cursor: default;
pointer-events: none;
}

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

.spectrum-Rating-starInactive {
display: none;
}
}
Comment on lines -102 to -111
Copy link
Copy Markdown
Collaborator

@marissahuysentruyt marissahuysentruyt Oct 17, 2024

Choose a reason for hiding this comment

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

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

}

.spectrum-Rating-input {
Expand Down Expand Up @@ -158,14 +148,14 @@
position: absolute;
}

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

@marissahuysentruyt marissahuysentruyt Oct 17, 2024

Choose a reason for hiding this comment

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

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?

.spectrum-Rating-starActive {
display: none;
display: block;
}

.spectrum-Rating-starInactive {
display: block;
display: none;
}
}
}
Expand All @@ -191,7 +181,7 @@
}

.spectrum-Rating--emphasized {
&.is-focused {
&.is-keyboardFocused {
.spectrum-Rating-icon.is-selected {
color: var(--highcontrast-rating-emphasized-icon-color-key-focus, var(--mod-rating-emphasized-icon-color-key-focus, var(--spectrum-rating-emphasized-icon-color-key-focus)));
}
Expand Down
21 changes: 12 additions & 9 deletions components/rating/metadata/metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
".spectrum-Rating--emphasized .spectrum-Rating-icon",
".spectrum-Rating--emphasized .spectrum-Rating-icon.is-selected",
".spectrum-Rating--emphasized .spectrum-Rating-icon:hover ~ .spectrum-Rating-icon",
".spectrum-Rating--emphasized.is-focused .spectrum-Rating-icon.is-selected",
".spectrum-Rating--emphasized.is-keyboardFocused .spectrum-Rating-icon.is-selected",
".spectrum-Rating--emphasized:hover .spectrum-Rating-icon",
".spectrum-Rating--emphasized:hover .spectrum-Rating-icon.is-currentValue:after",
".spectrum-Rating--emphasized:hover .spectrum-Rating-icon:active",
Expand All @@ -15,28 +15,31 @@
".spectrum-Rating-icon .spectrum-Rating-starActive",
".spectrum-Rating-icon .spectrum-Rating-starInactive",
".spectrum-Rating-icon.is-currentValue:after",
".spectrum-Rating-icon.is-hoverSelection .spectrum-Rating-starActive",
".spectrum-Rating-icon.is-hoverSelection .spectrum-Rating-starInactive",
".spectrum-Rating-icon.is-selected",
".spectrum-Rating-icon.is-selected .spectrum-Rating-starActive",
".spectrum-Rating-icon.is-selected .spectrum-Rating-starInactive",
".spectrum-Rating-icon:hover .spectrum-Rating-starActive",
".spectrum-Rating-icon:hover .spectrum-Rating-starInactive",
".spectrum-Rating-icon:hover ~ .spectrum-Rating-icon",
".spectrum-Rating-icon:hover ~ .spectrum-Rating-icon .spectrum-Rating-starActive",
".spectrum-Rating-icon:hover ~ .spectrum-Rating-icon .spectrum-Rating-starInactive",
".spectrum-Rating-input",
".spectrum-Rating-starActive",
".spectrum-Rating-starInactive",
".spectrum-Rating.is-disabled",
".spectrum-Rating.is-disabled .spectrum-Rating-icon",
".spectrum-Rating.is-disabled .spectrum-Rating-icon.is-selected",
".spectrum-Rating.is-focused",
".spectrum-Rating.is-focused .spectrum-Rating-icon",
".spectrum-Rating.is-focused .spectrum-Rating-icon.is-selected",
".spectrum-Rating.is-keyboardFocused",
".spectrum-Rating.is-keyboardFocused .spectrum-Rating-icon",
".spectrum-Rating.is-keyboardFocused .spectrum-Rating-icon.is-selected",
".spectrum-Rating.is-readOnly",
".spectrum-Rating:has(:focus-visible)",
".spectrum-Rating:has(:focus-visible) .spectrum-Rating-icon",
".spectrum-Rating:has(:focus-visible) .spectrum-Rating-icon.is-selected",
".spectrum-Rating:hover .spectrum-Rating-icon",
".spectrum-Rating:hover .spectrum-Rating-icon.is-currentValue:after",
".spectrum-Rating:hover .spectrum-Rating-icon:active",
".spectrum-Rating:hover .spectrum-Rating-icon:hover",
".spectrum-Rating:hover .spectrum-Rating-starActive",
".spectrum-Rating:hover .spectrum-Rating-starInactive"
".spectrum-Rating:hover .spectrum-Rating-icon:hover"
],
"modifiers": [
"--mod-rating-border-radius",
Expand Down
12 changes: 7 additions & 5 deletions components/rating/stories/rating.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { disableDefaultModes } from "@spectrum-css/preview/modes";
import { isDisabled, isEmphasized, isFocused, isReadOnly } from "@spectrum-css/preview/types";
import { isDisabled, isEmphasized, isKeyboardFocused, isReadOnly } from "@spectrum-css/preview/types";
import metadata from "../metadata/metadata.json";
import packageJson from "../package.json";
import { RatingGroup } from "./rating.test.js";
Expand All @@ -8,17 +8,18 @@ import { Template } from "./template.js";
/**
* The rating component is used to display or collect a user's rating of an item as represented by a number of stars.
*
* ### Usage notes
* ## Usage notes
* - All active stars have the class `is-selected` applied.
* - The current value (the last active star) has the class `is-currentValue` applied.
* - When the rating receives focus, the class `is-focused` should be added to the component's root element (`.spectrum-Rating`).
* - When the rating receives keyboard focus, the class `.is-keyboardFocused` should be added to the component's root element (`.spectrum-Rating`).
* - When the rating is hovered, the class `.is-hoverSelection` should be added to the `.spectrum-Rating-icon` being hovered over.
*/
export default {
title: "Rating",
component: "Rating",
argTypes: {
isEmphasized,
isFocused,
isKeyboardFocused,
isDisabled,
isReadOnly,
max: {
Expand Down Expand Up @@ -49,6 +50,7 @@ export default {
isDisabled: false,
isEmphasized: false,
isReadOnly: false,
isKeyboardFocused: false,
max: 5,
value: 3,
},
Expand All @@ -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.
Copy link
Copy Markdown
Collaborator

@marissahuysentruyt marissahuysentruyt Oct 16, 2024

Choose a reason for hiding this comment

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

👏 love a typo fix

* When hovering over a rating component that has a previously entered value, an underline appears under the
* current selection to provide context.
*/
Expand Down
4 changes: 2 additions & 2 deletions components/rating/stories/rating.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ export const RatingGroup = Variants({
isDisabled: true,
},
{
testHeading: "Focused",
isFocused: true,
testHeading: "Keyboard focused",
Copy link
Copy Markdown
Collaborator

@marissahuysentruyt marissahuysentruyt Oct 17, 2024

Choose a reason for hiding this comment

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

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.

isKeyboardFocused: true,
},
{
testHeading: "Read-only",
Expand Down
69 changes: 64 additions & 5 deletions components/rating/stories/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,89 @@ export const Template = ({
max = 5,
value = 0,
isReadOnly = false,
isFocused = false,
isKeyboardFocused = false,
isDisabled = true,
isEmphasized = false,
customClasses = [],
id = getRandomId("rating"),
} = {}, context = {}) => {
const { updateArgs } = context;
document.addEventListener("DOMContentLoaded", function() {
const rating = document.getElementById(id);
if (!rating) return;
if (rating.classList.contains("is-disabled") || rating.classList.contains("is-readOnly")) return;
const icons = Array.from(rating.getElementsByClassName("spectrum-Rating-icon"));
let hoverIndex = -1;
let selectedIndex = -1;

const updateHoverState = () => {
icons.forEach((icon, index) => {
const activeStar = icon.querySelector(".spectrum-Rating-starActive");
const inactiveStar = icon.querySelector(".spectrum-Rating-starInactive");

if (index <= hoverIndex ||
(index <= selectedIndex && hoverIndex === -1)
) {
icon.classList.add("is-hoverSelection");
activeStar.style.display = "block";
inactiveStar.style.display = "none";
}
Comment on lines +35 to +41
Copy link
Copy Markdown
Collaborator

@marissahuysentruyt marissahuysentruyt Oct 17, 2024

Choose a reason for hiding this comment

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

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 🤦‍♀️

else {
icon.classList.remove("is-hoverSelection");
activeStar.style.display = "none";
inactiveStar.style.display = "block";
}
});
};

rating.addEventListener("mouseleave", function() {
icons.forEach(icon => {
icon.classList.remove("is-hoverSelection");
icon.querySelector(".spectrum-Rating-starActive").style.display = "";
icon.querySelector(".spectrum-Rating-starInactive").style.display = "";
});
});

icons.forEach((icon, index) => {
if (icon.classList.contains("is-selected")) selectedIndex = index;

icon.addEventListener("mouseover", function() {
hoverIndex = index;
updateHoverState();
});

icon.addEventListener("mouseleave", function(event) {
if (!rating.contains(event.relatedTarget)) {
Copy link
Copy Markdown
Collaborator

@marissahuysentruyt marissahuysentruyt Oct 17, 2024

Choose a reason for hiding this comment

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

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

hoverIndex = -1;
updateHoverState();
}
});

icon.addEventListener("click", function() {
selectedIndex = index;
updateHoverState();
});
});

updateHoverState();
});

return html`
<div
class=${classMap({
[rootClass]: true,
"is-disabled": isDisabled,
"is-readOnly": isReadOnly,
"is-focused": isFocused,
"is-keyboardFocused": isKeyboardFocused,
Comment thread
marissahuysentruyt marked this conversation as resolved.
[`${rootClass}--emphasized`]: isEmphasized,
...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}),
})}
id=${ifDefined(id)}
@focusin=${function() {
updateArgs({ isFocused: true });
updateArgs({ isKeyboardFocused: true });
}}
@focusout=${function() {
updateArgs({ isFocused: false });
updateArgs({ isKeyboardFocused: false });
}}
>
<input
Expand Down Expand Up @@ -69,7 +128,7 @@ export const Template = ({
"is-currentValue": idx === value - 1,
})}
@click=${function() {
updateArgs({ value: idx + 1, isFocused: true });
updateArgs({ value: idx + 1 });
}}
>
${Icon({
Expand Down
25 changes: 18 additions & 7 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7413,7 +7413,18 @@ __metadata:
languageName: node
linkType: hard

"cross-spawn@npm:^7.0.0, cross-spawn@npm:^7.0.1, cross-spawn@npm:^7.0.2, cross-spawn@npm:^7.0.3, cross-spawn@npm:^7.0.5":
"cross-spawn@npm:^7.0.0, cross-spawn@npm:^7.0.1, cross-spawn@npm:^7.0.2, cross-spawn@npm:^7.0.3":
version: 7.0.3
resolution: "cross-spawn@npm:7.0.3"
dependencies:
path-key: "npm:^3.1.0"
shebang-command: "npm:^2.0.0"
which: "npm:^2.0.1"
checksum: 10c0/5738c312387081c98d69c98e105b6327b069197f864a60593245d64c8089c8a0a744e16349281210d56835bb9274130d825a78b2ad6853ca13cfbeffc0c31750
languageName: node
linkType: hard

"cross-spawn@npm:^7.0.5":
version: 7.0.6
resolution: "cross-spawn@npm:7.0.6"
dependencies:
Expand Down Expand Up @@ -8789,13 +8800,13 @@ __metadata:
linkType: hard

"eslint-compat-utils@npm:^0.6.0":
version: 0.6.4
resolution: "eslint-compat-utils@npm:0.6.4"
version: 0.6.3
resolution: "eslint-compat-utils@npm:0.6.3"
dependencies:
semver: "npm:^7.5.4"
peerDependencies:
eslint: ">=6.0.0"
checksum: 10c0/5b665c4051e978b9f9c48621f63d07e6b2a8ba1b334fc430f1ce0d8b596968677bdb54c23c00ca961ad5b4673d5e83e014a52b4baf9a2f7d4ccd79e3c213acfb
checksum: 10c0/5015cd92590ab4630dfddc4416b058c2e40c58c548203761745e6874bf3e06f15dd84fc447187853025924f86d1870efc959032fcffaf5003e32a7d4f822c43f
languageName: node
linkType: hard

Expand Down Expand Up @@ -12289,11 +12300,11 @@ __metadata:
linkType: hard

"nanoid@npm:^3.3.7":
version: 3.3.8
resolution: "nanoid@npm:3.3.8"
version: 3.3.7
resolution: "nanoid@npm:3.3.7"
bin:
nanoid: bin/nanoid.cjs
checksum: 10c0/4b1bb29f6cfebf3be3bc4ad1f1296fb0a10a3043a79f34fbffe75d1621b4318319211cd420549459018ea3592f0d2f159247a6f874911d6d26eaaadda2478120
checksum: 10c0/e3fb661aa083454f40500473bb69eedb85dc160e763150b9a2c567c7e9ff560ce028a9f833123b618a6ea742e311138b591910e795614a629029e86e180660f3
languageName: node
linkType: hard

Expand Down