Use CSS for colors of VWordPress and VHomeLink #5110

obulat · 2024-10-29T13:25:47Z

Fixes

Description

This PR adds CSS variables for the contrasting text color and for the focus ring color so that the logo elements (VWordPressLink and VHomeLink) do not use JS to determine colors, even when the colors don't match the current color mode (i.e., on the pages modal, which has black background both in the dark and in the light themes).

To prevent some of the focus styles overriding each other, I changed focus-slim-tx class to only apply on focus-visible. Otherwise, it changes the ring color not only for the element in question, but for child elements, and sometimes is difficult to override.

Testing Instructions

Run the app using ov just frontend/run dev
Open http://localhost:8443 in a narrow window, and click on the menu button in the top right corner. Use the keyboard to navigate though the contents. The focus rings should be yellow, in dark and in light themes. The WordPress and Openverse links should have correct colors.

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
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.

github-actions · 2024-10-29T14:17:01Z

Latest k6 run output¹

     ✓ status was 200

     checks.........................: 100.00% ✓ 402      ✗ 0   
     data_received..................: 95 MB   396 kB/s
     data_sent......................: 52 kB   217 B/s
     http_req_blocked...............: avg=38.36µs  min=2.43µs   med=4.41µs   max=1.19ms p(90)=147.49µs p(95)=174.29µs
     http_req_connecting............: avg=25.43µs  min=0s       med=0s       max=1.16ms p(90)=100.51µs p(95)=117.33µs
     http_req_duration..............: avg=199.35ms min=30.1ms   med=159.07ms max=1.31s  p(90)=378.98ms p(95)=421.69ms
       { expected_response:true }...: avg=199.35ms min=30.1ms   med=159.07ms max=1.31s  p(90)=378.98ms p(95)=421.69ms
   ✓ http_req_failed................: 0.00%   ✓ 0        ✗ 402 
     http_req_receiving.............: avg=161.75µs min=58.59µs  med=133.83µs max=1.06ms p(90)=278.4µs  p(95)=330.57µs
     http_req_sending...............: avg=23.54µs  min=9.19µs   med=21.46µs  max=75.9µs p(90)=35.96µs  p(95)=42.8µs  
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s     p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=199.17ms min=29.99ms  med=158.95ms max=1.31s  p(90)=378.48ms p(95)=421.34ms
     http_reqs......................: 402     1.669538/s
     iteration_duration.............: avg=1.07s    min=359.71ms med=1.18s    max=2.42s  p(90)=1.38s    p(95)=1.73s   
     iterations.....................: 75      0.311481/s
     vus............................: 1       min=0      max=6 
     vus_max........................: 60      min=60     max=60

This comment will automatically update with new output each time k6 runs for this PR ↩

github-actions · 2024-10-29T14:20:25Z

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

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.

AetherUnbound

When I run this locally, it looks like the WordPress, Openverse, and Exit buttons on the menu are all either pink on light mode, or yellow on dark mode. Is that what's expected? Between my local copy and staging, the only difference I can tell is that the WordPress logo goes from yellow on light mode to pink on light mode.

AetherUnbound · 2024-11-08T06:14:52Z

Drafting until questions can be addressed.

obulat · 2024-12-12T10:02:38Z

When I run this locally, it looks like the WordPress, Openverse, and Exit buttons on the menu are all either pink on light mode, or yellow on dark mode. Is that what's expected? Between my local copy and staging, the only difference I can tell is that the WordPress logo goes from yellow on light mode to pink on light mode.

The focus rings are expected to be yellow on the dark background, and pink on the light background.

In the menu modal that has black background, the focus ring should always be yellow, both in the dark and in the light theme.

The background shoud not change on hover (it does in production in the dark theme currently)

AetherUnbound · 2024-12-12T23:00:17Z

On light mode, I'm still seeing the Openverse logo, X, and WP logo at the bottom highlighted in purple within the modal menu 😕 It does change to yellow in dark mode, though.

dhruvkb · 2024-12-13T05:09:15Z

frontend/src/styles/tailwind.css

@@ -565,6 +570,8 @@ Time                - 11px - xs semibold (default leading-tight)

    @apply after:pointer-events-none after:absolute after:inset-0 after:rounded-inherit;
    @apply after:z-10 after:shadow-bold-filled; /* inner-stroke: pseudo-element */
+
+    @apply ring-[--color-focus-ring]; /* default ring color */


Wouldn't this work (since you already have the color defined in Tailwind config)?

Suggested change

@apply ring-[--color-focus-ring]; /* default ring color */

@apply ring-focus-ring; /* default ring color */

In fact one could only define the new color --color-focus-rings inside ringColor so that it can be written as:

ringColor: { focus: 'var(--color-focus-ring)', // ... }

@apply ring-focus; /* ... */

Thank you, @dhruvkb , I my understanding of how the CSS variables work with Tailwind was wrong.
I added ring-focus and outline-focus.

obulat · 2024-12-13T10:12:27Z

frontend/src/components/VModal/VModalContent.vue

@@ -122,7 +122,9 @@ defineExpose({
        v-bind="$attrs"
        class="flex flex-col"
        :class="[
-          mode === 'dark' ? 'bg-black text-default' : 'bg-overlay text-default',
+          mode === 'dark'
+            ? 'dark-mode bg-black text-default'


The dark mode VModalContent is manually set to dark-mode so that the focus rings are yellow and don't change with the sites main color mode.

openverse-bot · 2024-12-25T00:00:09Z

Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR:

@krysal
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 6 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)².

@obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

obulat requested a review from a team as a code owner October 29, 2024 13:25

obulat requested review from krysal and dhruvkb October 29, 2024 13:25

obulat marked this pull request as draft October 29, 2024 13:58

obulat self-assigned this Oct 29, 2024

obulat marked this pull request as ready for review October 29, 2024 14:37

obulat force-pushed the fix/css-for-colors branch from a7f134f to d894e3d Compare October 31, 2024 11:32

AetherUnbound reviewed Oct 31, 2024

View reviewed changes

AetherUnbound marked this pull request as draft November 8, 2024 06:14

obulat force-pushed the fix/css-for-colors branch 2 times, most recently from 17f37d7 to 524e60d Compare December 4, 2024 06:21

obulat force-pushed the fix/css-for-colors branch 2 times, most recently from 788e44f to 157391a Compare December 12, 2024 09:47

obulat marked this pull request as ready for review December 12, 2024 10:02

dhruvkb reviewed Dec 13, 2024

View reviewed changes

obulat force-pushed the fix/css-for-colors branch 2 times, most recently from 962a0d9 to fa0ee8f Compare December 13, 2024 08:40

obulat commented Dec 13, 2024

View reviewed changes

obulat marked this pull request as draft December 13, 2024 12:48

obulat force-pushed the fix/css-for-colors branch from e11497b to 0da15bd Compare December 15, 2024 21:24

obulat marked this pull request as ready for review December 16, 2024 03:26

obulat added 6 commits December 17, 2024 16:57

Use CSS for colors of VWordPress and VHomeLink

0f05893

Fix modal background and uncommon ring color

42b38db

Lint

69a1149

Re-add focus

c1d1d43

Lint

eb3e42f

Fix missing focus-visible:

3dff313

obulat force-pushed the fix/css-for-colors branch from 8e94de2 to 3dff313 Compare December 17, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CSS for colors of VWordPress and VHomeLink #5110

Use CSS for colors of VWordPress and VHomeLink #5110

obulat commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024

AetherUnbound left a comment

AetherUnbound commented Nov 8, 2024

obulat commented Dec 12, 2024

AetherUnbound commented Dec 12, 2024

dhruvkb Dec 13, 2024

obulat Dec 13, 2024

obulat Dec 13, 2024

openverse-bot commented Dec 25, 2024

	@apply ring-[--color-focus-ring]; /* default ring color */
	@apply ring-focus-ring; /* default ring color */

Use CSS for colors of VWordPress and VHomeLink #5110

Are you sure you want to change the base?

Use CSS for colors of VWordPress and VHomeLink #5110

Conversation

obulat commented Oct 29, 2024

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

github-actions bot commented Oct 29, 2024 • edited Loading

Latest k6 run output1

Footnotes

github-actions bot commented Oct 29, 2024

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound commented Nov 8, 2024

obulat commented Dec 12, 2024

AetherUnbound commented Dec 12, 2024

dhruvkb Dec 13, 2024

Choose a reason for hiding this comment

obulat Dec 13, 2024

Choose a reason for hiding this comment

obulat Dec 13, 2024

Choose a reason for hiding this comment

openverse-bot commented Dec 25, 2024

Footnotes

github-actions bot commented Oct 29, 2024 •

edited

Loading

Latest k6 run output¹