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

Update appearance of sensitive image thumbnails #4831

Merged
merged 10 commits into from
Sep 6, 2024
Merged

Conversation

dhruvkb
Copy link
Member

@dhruvkb dhruvkb commented Aug 28, 2024

Fixes

Fixes #4777 by @fcoveram

Description

This PR

  • adds a new background token "blur" that corresponds to white-88 or black-72 depending on the theme
    • removes an incorrect, and redundant, use of bg-blur
  • updates the image cell to show the text "Sensitive content" and an icon indicating that the image has been blurred because of sensitivity

Testing Instructions

  1. Check out the PR and run the Nuxt server.
  2. Go to /preferences and check the "fake_sensitive" preference.
  3. Perform a search for something.
  4. Verify that a number of results have been blurred and show the "hide" icon.
  5. Hover or focus over these results to reveal the bottom-centered "Sensitive content" note instead of the license info.
  6. Switch to a mobile view.
  7. Verify that the results have the "hide" icon and show the "Sensitive content" note in place of the license info.

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • I ran the DAG documentation generator (ov just catalog/generate-docs for catalog
    PRs) or the media properties generator (ov just catalog/generate-docs media-props
    for the catalog or ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@openverse-bot openverse-bot added 🧱 stack: documentation Related to Sphinx documentation 🧱 stack: frontend Related to the Nuxt frontend 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🕹 aspect: interface Concerns end-users' experience with the software labels Aug 28, 2024
Copy link

Full-stack documentation: https://docs.openverse.org/_preview/4831

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

@dhruvkb dhruvkb marked this pull request as ready for review August 28, 2024 18:07
@dhruvkb dhruvkb requested a review from a team as a code owner August 28, 2024 18:07
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

  • I don't think we can nest <div> in an <a>, so perhaps it would be better to wrap the icons and label in <span>s
  • The text on hover is inheriting an underline from the parent <a> tag
  • The icon is the wrong color in dark mode (looks like there is a fill hardcoded on the svg path)

The updated blurs look gorgeous!

Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

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

Nice! ✨
Two problems I found:

  1. There is a 1px non-blurred stripe at the bottom of the image:
Screenshot 2024-08-28 at 10 06 57 PM 2. The eye icon in the dark theme is not visible, should be of a different color.

@dhruvkb dhruvkb marked this pull request as draft August 29, 2024 04:52
@dhruvkb
Copy link
Member Author

dhruvkb commented Aug 29, 2024

I don't think we can nest <div> in an <a>

@zackkrida, I thought so too, but I just learned that in HTML5 <a> now allows <div>s. Nonetheless, switched to <span> in 3e057ba.

There is a 1px non-blurred stripe at the bottom of the image

@obulat I tried both Firefox and Chrome but I am not able to reproduce the 1px line at the bottom, can you share your browser info?

@dhruvkb dhruvkb marked this pull request as ready for review August 29, 2024 05:47
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

Just two more small pieces of feedback, one we likely have to ask @fcoveram about.

  1. I think we should remove the title attribute from the anchor tag, as the tooltip feels redundant:
    Screenshot from 2024-08-29 07-28-01

  2. The light mode overlay color is very overpowering, and removes much of the blurred quality and the underlying image:

With
Screenshot from 2024-08-29 07-36-07

Without:
Screenshot from 2024-08-29 07-36-01

To the point that it almost looks like a plain grey background. I wonder if it's possible to adjust that?

@fcoveram
Copy link
Contributor

To your points:

  1. Agree. It is redundant.
  2. You are right. I lean on keeping the blur layer in a light tone to then shift it to dark in dark mode. We could try #ffffff at 80%, 72%, and 64%, and see which one feels better. Once we decided the % value, I update the Figma variable.

How does that sound?

@obulat
Copy link
Contributor

obulat commented Aug 30, 2024

@obulat I tried both Firefox and Chrome but I am not able to reproduce the 1px line at the bottom, can you share your browser info?

This must have something to do with the size calculations in the image grid. It only happens on the image grid where the sizes are variable. I could reproduce it in Chrome and Safari. Here's a Safari screenshot. Lines at the bottom are not so common, but the corners often have a different-color dot. I don't want to block on this because I think it won't be that noticeable.

Safari screenshot, see the first image in the row:

Screenshot 2024-08-30 at 5 49 36 PM

Chrome screenshot, look at the bottom row:

Screenshot 2024-08-30 at 5 47 21 PM

@dhruvkb
Copy link
Member Author

dhruvkb commented Aug 30, 2024

Dropped the tooltip in 92031f9.

@fcoveram would you like to see some screenshots at the different opacity levels?

@obulat I am still not able to see the lines. I'll keep trying to get them to show up, and will fix them in post.

@dhruvkb
Copy link
Member Author

dhruvkb commented Aug 30, 2024

Update: I got the line to show up! It seems like a strange rendering issue because the dimensions and position of both elements is exactly the same upto 5 decimal places.

Screenshot 2024-08-30 at 8 58 01 PM

@fcoveram
Copy link
Contributor

fcoveram commented Sep 2, 2024

@fcoveram would you like to see some screenshots at the different opacity levels?

That sounds great. Thanks @dhruvkb

@dhruvkb
Copy link
Member Author

dhruvkb commented Sep 2, 2024

image Screenshot 2024-09-02 at 8 06 36 PM

@fcoveram these are 88% (current), 80%, 72%, 64% and no overlay respectively. As @zackkrida mentioned the current version shows almost no features of the underlying image, which only start to appear at much lower opacity (even 64% might be too much).

Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

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

The icon color was fixed, and the lines appear very rarely, and can be fixed when we find the reason for them. Nice improvement ✨

@dhruvkb
Copy link
Member Author

dhruvkb commented Sep 3, 2024

I will merge this PR once @fcoveram's preferred opacity level is implemented.

@zackkrida
Copy link
Member

@dhruvkb I think 72% might be the best option, while we await Francisco's opinion 😆 Thank you for showing the different options.

@fcoveram
Copy link
Contributor

fcoveram commented Sep 6, 2024

I agree that 72% looks good.

I simulated having an image with pure black area beneath the sensitive content text to see if it meet a11y requirements, and it seems safe to use. Here is a screenshot

Screenshot of image component with contrast requirements for accessibility notes on the right

So +1 to go with 72%

@dhruvkb dhruvkb dismissed zackkrida’s stale review September 6, 2024 08:56

Implemented feedback

@dhruvkb dhruvkb merged commit 2186477 into main Sep 6, 2024
45 checks passed
@dhruvkb dhruvkb deleted the sensitive_image_improv branch September 6, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕹 aspect: interface Concerns end-users' experience with the software ✨ goal: improvement Improvement to an existing user-facing feature 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: documentation Related to Sphinx documentation 🧱 stack: frontend Related to the Nuxt frontend
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improvement over image component
5 participants