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

#2840: Updated gallery cards for label types that do not support severity ratings #2954

Merged
merged 14 commits into from
Aug 8, 2022

Conversation

davphan
Copy link
Collaborator

@davphan davphan commented Jul 1, 2022

Resolves #2840

Changed Gallery cards with unsupported severity ratings to have grayed out severity ratings, as well as a tooltip that, wehn hovered over, explains why they have no rating.

Before/After screenshots (if applicable)

Before:
image

After:
image

Testing instructions
  1. Go to the gallery tab
  2. Under the "Show" drop down on the left, hit "Pedestrian Signal," or any label type that does not support severity ratings. (if the server you're working on doesn't have any pedestrian signals tagged, go into the audit tab and throw in a couple random pedestrian signal labels, not in production server ofc!!!)
  3. Check that the Severity Rating is grayed out on all cards, and that a tooltip is there. When hovered over, a message box should appear.
  4. Check in all languages!
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 asked for and included translations for any user facing text that was added or modified.

… out severity rating and hoverable tooltip that explains why they have no rating
@misaugstad
Copy link
Member

I think that we can skip out on the '?' icon and just have the tooltip on the grayed out section. Do you have this on the expanded view when you click on a card as well?

…tip and added it to the modal card popup as well, cleaned up redundant code
@davphan
Copy link
Collaborator Author

davphan commented Jul 5, 2022

Since we dont have a grayed out image for the smiley faces in the expanded view I just made them grayed out circles when there's no severity rating, does that work?

@misaugstad
Copy link
Member

There's this CSS property called filter that you can apply to an image. I just tested out setting filter: opactity(0.5) to one of the smileys and it looks like it's grayed out 👌
Screenshot from 2022-07-05 12-33-26

@misaugstad
Copy link
Member

I think that what you did is make it so that whenever a label doesn't have severity, you show the "unsupported" tooltip. But we actually only wanted to show this tooltip for the two label types where you aren't even able to add a severity rating: "Can't see the sidewalk" and "Pedestrian signal".

This does make me realize that we should do the grayed out with a tooltip for other label types if severity isn't provided as well. But that tooltip should just say that the user did not provide a severity rating.

In addition, the Can't See the Sidewalk label type is still showing no severity rating at all!

Here's screenshots with the various bits of incorrect behavior:
Screenshot from 2022-07-06 13-01-40
Screenshot from 2022-07-06 13-03-28
Screenshot from 2022-07-06 13-02-09

@misaugstad
Copy link
Member

Here's the one thing from the list above that I think you missed!

This does make me realize that we should do the grayed out with a tooltip for other label types if severity isn't provided as well. But that tooltip should just say that the user did not provide a severity rating.

@misaugstad
Copy link
Member

A few more notes!

  1. I made an update myself to remove old severity ratings for Pedestrian Signal labels since they were allowed for a couple weeks there. The existence of that data was causing bugs here, and I figured that wasn't worth it. So I cleaned up my old mess :)
  2. Can you change this text slightly to say "A severity rating was not provided for this label"
    Screenshot from 2022-07-19 13-52-35
  3. The expanded view tooltip is off center
    Screenshot from 2022-07-19 12-38-50
  4. The grayed out text looks really bad on validated labels. This is because you're changing the font color to lightgray. You should probably just be keeping it as black and reducing the opacity.
    Screenshot from 2022-07-19 13-52-40

Those last two should have become clear with extensive testing, so I challenge you to step up your code testing game in future iterations and PRs! ;)

@davphan
Copy link
Collaborator Author

davphan commented Jul 28, 2022

@misaugstad Ready for another review!

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.

Just added some super minor style changes. Everything is working great though! Just about ready to merge!

public/javascripts/Gallery/css/cards.css Outdated Show resolved Hide resolved
public/javascripts/Gallery/src/displays/SeverityDisplay.js Outdated Show resolved Hide resolved
public/javascripts/Gallery/src/displays/SeverityDisplay.js Outdated Show resolved Hide resolved
public/javascripts/Gallery/src/displays/SeverityDisplay.js Outdated Show resolved Hide resolved
public/javascripts/Gallery/src/displays/SeverityDisplay.js Outdated Show resolved Hide resolved
@davphan
Copy link
Collaborator Author

davphan commented Aug 5, 2022

Ready for another check!

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.

Looks good, thanks @GPA0VERKD :)

@misaugstad misaugstad merged commit d02cf45 into develop Aug 8, 2022
@misaugstad misaugstad deleted the 2840-gray-severity-levels branch August 9, 2022 01:53
@misaugstad misaugstad mentioned this pull request Aug 19, 2022
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.

Gray out severity levels for unsupported label types
2 participants