-
Notifications
You must be signed in to change notification settings - Fork 202
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 link focus rings #4958
Fix link focus rings #4958
Conversation
8940e6b
to
8a1e648
Compare
Full-stack documentation: https://docs.openverse.org/_preview/4958 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. |
e9634bb
to
4981994
Compare
focus-slim-offset
styles2a465ec
to
ea74eb4
Compare
9ee8226
to
cc875c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obulat @zackkrida are there plans to refactor the WP logo component to use the CSS-only methods of colour scheme detection? I suppose it retains the JS approach in order to support the mobile nav, which is always in dark mode... but could the mobile nav add the dark mode classname to its root element, and then follow the same approach for colours as the rest of the application? It will be confusing long term to have the JS-based pattern still present in the app, for anyone trying to understand how to correctly style components.
The mobile pages modal is always black, in dark and in light theme. We use a CSS variable to set the background color of the current page, and in this modal, we set it to black (even though in the main app it can be yellow on the homepage for light, and dark gray for dark theme). We could set a similar "contrast text color" variable when we set the page background color variable for the colors of the WordPress link and Openverse logo. Would that be a better approach? |
a63826b
to
bd3edf8
Compare
@sarayourfriend that's a great idea. @obulat is that suggestion makes sense to you as well could you create an issue for it? |
bd3edf8
to
252b22c
Compare
Latest k6 run output1
Footnotes
|
Issue opened: #4998 |
6b7af2f
to
49d0109
Compare
6bbb734
to
b2ec51f
Compare
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was ready for review 10 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
5a401e9
to
b9a0b59
Compare
b9a0b59
to
4bbd715
Compare
6f223aa
to
815669b
Compare
815669b
to
628b474
Compare
These changes were merged with #4983. |
Fixes
Related to #4911 by @zackkrida
Description
This PR fixes most of the focus ring problems for
VLink
elements.Most of
VLink
elements use the defaultoutline
that the browser automatically activates on focus. This PR correctly sets the color of the outline. In Chrome, the defaultauto
outline style generates two-tiered outline (dark/light), to make sure it works in both dark and light themes). However, since we set the color of the outline with an appropriate contrast, we can use a single-color outline. To do that, we set the default outline style forfocus-visible
tosolid
, which creates a ring of single color.To make the outline rounded, if a
VLink
does not set arounded-
class, then we addrounded-sm
(border-radius: 2px) to it.Testing Instructions
ov just frontend/run dev
http://localhost:8443/
and navigate around using Tab. The links in the header/footer, Openverse link and the WordPress link, as well as other links throughout the site should have a rounded outline of correct color: pink in light theme, and yellow in dark theme.Use the Storybook to test the links, e.g., Internal header: https://docs.openverse.org/_preview/4958/storybook/?path=/story/components-vheader-vheaderinternal--default&globals=viewport:xs
Note that in the light mode, the Openverse link and the close button in the modal still has a pink focus outline, it will be fixed in a later PR.
Check different browsers.
This PR also contains a fix to an e2e test that failed twice in the CI. The fix was to check that the sensitive switch was indeed changed before checking for the cookies.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) 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