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

2640 redesigned smiley scale #3097

Merged
merged 19 commits into from
Jan 6, 2023
Merged

Conversation

hoominchu
Copy link
Collaborator

@hoominchu hoominchu commented Dec 18, 2022

Resolves #2640

This is a brief description of the problem solved or feature implemented and how you implemented it.

The smileys used on gallery, validate, and audit page started from a 'smiling face' which needed a redesign to start from a 'neutral face'. Additionally the icons were not consistent and used PNGs instead of SVGs—both of these issues are addressed.

Before/After screenshots (if applicable)

The smiley redesign on the filter section of the gallery page.
Before
Screenshot 2022-12-18 at 9 17 52 AM

After
image

More screenshots here: #2640 (comment)

Testing instructions
  1. Ensure that all the smileys on gallery, validate (both desktop and mobile), and audit pages are updated and consistent.
  2. Ensure that all the smiley interactions on the above pages work as expected. No change in behavior is expected.
Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.
  • I've tested on mobile (only needed for validation page).

@misaugstad
Copy link
Member

Just two comments for now from testing. I'll take a look at the code next!

I like the little fade in animation when you select a severity on the Explore page, but it sticks around when you open up a new context menu, and you see the fade out to no severity or a different severity. GIF below. It's not a huge deal, but I did find it a bit visually distracting to see the color change after opening the menu each time.
Peek 2022-12-29 16-21

Something else that might be nice to have, and I'm interested to hear what @jonfroehlich and @hoominchu think about this: When choosing a severity on the sidebar in Gallery, should we make it so that all of the smileys to the left of the one you're hovering over are also filled in?

@misaugstad
Copy link
Member

Code looks great! Though I didn't really understand why we need to duplicate the svg code in audit.scala.html, but not gallery.scala.html.

@hoominchu
Copy link
Collaborator Author

hoominchu commented Jan 4, 2023

@misaugstad pushed a fix for the delay in the smiley animation when a new context menu is opened.

Re: svg code being duplicated in audit.scala.html and not in gallery.scala.html, on the audit page, we need smileys to be drawn inside the canvas and for this we need the 'fill' values to be hardcoded in the SVGs. This is not a requirement in any of the other pages. Let me know if you would like to discuss this on a call.

Re: highlighting all the smileys to the left of the hovered one in gallery sidebar, I wonder if that conveys that all the severity levels to the left will be selected while we actually select only the one hovered/clicked upon?

@misaugstad
Copy link
Member

Re: highlighting all the smileys to the left of the hovered one in gallery sidebar, I wonder if that conveys that all the severity levels to the left will be selected while we actually select only the one hovered/clicked upon?

I think that's fair. I think that both methods are confusing in their own ways, and the way I suggested would take a nontrivial amount of effort, so let's stick with what we've got :)

Hoping to take a final look later today!

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looking good!! The transition is still a bit weird, because now it only does the slow transition when using keyboard shortcuts if your mouse is over the context menu... But I'm in the middle of greatly simplifying the JS that relates to that stuff, so it might be easier for me to make that quick change later!

@misaugstad misaugstad merged commit 78ba162 into develop Jan 6, 2023
@misaugstad misaugstad deleted the 2640-redesigned-smiley-scale branch January 6, 2023 20:32
@jonfroehlich
Copy link
Member

Thanks @misaugstad. Can you make a new Issue for that so we don't forget?

Awesome to see this come to fruition. Great work @hoominchu and @misaugstad!!

@misaugstad misaugstad mentioned this pull request Jan 6, 2023
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.

Redesign smiley severity scale to start at a neutral face
3 participants