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

fix(placement): adjust hover style for links #9981

Merged
merged 9 commits into from
May 2, 2024

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Nov 8, 2023

Summary

Problem

The "Mozilla ads" and "Don't want to see ads?" are not always recognizable as links, because they don't always change their text-decoration on hover.

Solution

Make sure that the text-decoration changes on hover.


Screenshots

Before

Screen.Recording.2023-11-30.at.14.33.11.mov

After

Screen.Recording.2023-11-30.at.14.34.40.mov

How did you test this change?

Ran yarn dev with REACT_APP_KUMA_HOST=developer.allizom.org and REACT_APP_PLACEMENT_ENABLED=true in my .env and looked at http://localhost:3000/en-US/docs/Web/HTML, then compared with main.

@caugner caugner changed the title fix(placement): add hover states for "Mozilla ads" and "Don't want to see ads?" fix(placement): add hover states for links Nov 8, 2023
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

code lgtm, I'll let you test when you add screenshots 🚀

@caugner
Copy link
Contributor Author

caugner commented Nov 30, 2023

Noticed two issues:

  1. The ad text in the sidebar gets underlined on hover (might actually be desired, but this change was not intended).
  2. The underline of the "Mozilla ads" text in the top banner does not toggle on hover.

edit: Fixed now.

@caugner caugner marked this pull request as ready for review November 30, 2023 13:42
@caugner caugner requested a review from a team as a code owner November 30, 2023 13:42
@caugner caugner requested a review from LeoMcA November 30, 2023 13:42
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

Code looks good, but a super opinionated view (so feel free to ignore) on the "Mozilla ads" underline (I think it looks bad):

client/src/ui/organisms/placement/index.scss Show resolved Hide resolved
@caugner caugner marked this pull request as draft November 30, 2023 19:33
@github-actions github-actions bot added the idle label Jan 24, 2024
@caugner caugner marked this pull request as ready for review May 2, 2024 22:29
Copy link
Contributor Author

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Self-approval, and tested again:

  1. In the top, both "Mozilla ads" and "Don't want to see ads?" are underlined only on hover.
  2. In the sidebar, "Mozilla ads" is underlined except on hover, and "Don't want to see ads?" is underlined only on hover.
  3. In the footer, "Mozilla ads" is underlined except on hover, and "Don't want to see ads?" is underlined only on hover.

Bottom line is this now works as expected, and code changes have already been reviewed by Leo.

@caugner caugner changed the title fix(placement): add hover states for links fix(placement): adjust hover states for links May 2, 2024
@caugner caugner merged commit 67a8e4c into main May 2, 2024
15 checks passed
@caugner caugner deleted the fix-hover-style-of-placement-links branch May 2, 2024 22:32
@caugner caugner changed the title fix(placement): adjust hover states for links fix(placement): adjust hover style for links May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants