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

Improve star highlighting behavior #24

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Improve star highlighting behavior #24

merged 6 commits into from
Nov 27, 2023

Conversation

AdrianaCeric
Copy link
Contributor

@AdrianaCeric AdrianaCeric commented Nov 2, 2023

Previously, hovered stars were a lighter yellow. This could be misinterpreted as a half-star. So, I am making the highlight on hover a darker yellow
Inspired by this tutorial's hovering behaviour.

Copy link

vercel bot commented Nov 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atlrides-survey ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 3:26pm
qa-survey ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 3:26pm
st-survey ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 3:26pm

@AdrianaCeric
Copy link
Contributor Author

AdrianaCeric commented Nov 2, 2023

Making this a CSS-only change (e.g., setting :hover to fill the star with yellow) was difficult for some reason. Also, instead of the boxed outline, would an underline make sense and maybe look cleaner re: #18

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

If you set the fill in the star svg to currentColor, you can set the color of the svg from the css, and use the :hover pseudo-class!

Could we look at some ways to accomplish what you're doing here (or something similar) with pure css? I don't mind the effect it creates, but I'd love to avoid a lot of states and props being introduced if we can avoid it!

@AdrianaCeric
Copy link
Contributor Author

I'm not sure how to achieve the same effect where previous stars are highlighted with purely css, but I think this solution is cleaner. Let me know!

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

This is getting there! I think the issue with this functionality is that there's no visible :hover on the yellow stars! I'd love to see a hover on both the selected and deselected stars. I've added some comments on a way to approach this, but let me know if you want to hop on a call and I can better explain!

components/Stars.tsx Outdated Show resolved Hide resolved
components/Stars.tsx Outdated Show resolved Hide resolved
styles/Stars.module.css Outdated Show resolved Hide resolved
@AdrianaCeric
Copy link
Contributor Author

Thanks for the feedback and help Amy! Right now I think the simplest way to go about this is to just highlight the hovered star a darker yellow. Let me know

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

Looking really good! I have a nitpicky suggestion for the colors on the stars/hover states, but happy to leave that up to you and the second reviewer! This works as-is

styles/Stars.module.css Outdated Show resolved Hide resolved
@AdrianaCeric AdrianaCeric removed their assignment Nov 16, 2023
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Great cleanup! Code looks much cleaner. Not sure how I feel about the dimmed star color, but it's a good start

styles/Stars.module.css Show resolved Hide resolved
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

I'd just use hex codes here rather than using a brightness filter twice. Also, right now the selected stars get darker, and the unselected stars get brighter, which is looking a little messy. Can we just do like:

.star {
color: #8C8C8C;
}

.star:hover {
color: #666666
}

.selected {
color: #ffda1f
}

.selected:hover {
color: #ffb300;
}

@amy-corson-ibigroup
Copy link
Contributor

I'd also add a transition on these just to make it a little smoother!

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Good enough for now!

styles/Stars.module.css Outdated Show resolved Hide resolved
@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Nov 23, 2023
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

Thank you!

@AdrianaCeric AdrianaCeric merged commit d25944f into dev Nov 27, 2023
6 checks passed
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.

3 participants