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

Dark Mode Frontend Implementation Plan #3963

Merged
merged 10 commits into from
May 1, 2024
Merged

Dark Mode Frontend Implementation Plan #3963

merged 10 commits into from
May 1, 2024

Conversation

zackkrida
Copy link
Member

@zackkrida zackkrida commented Mar 25, 2024

Fixes

Fixes #3958 by @zackkrida

Description

This is an implementation plan for adding a dark mode to the Openverse frontend as part of the #3592 project.

I have chosen @obulat and @sarayourfriend as reviewers for their expertise in implementing frontend features, TypeScript generally, and familiarity with the testing-related code in the frontend.

Testing Instructions

Please read the IP using the decision-making process described below.

Decision-making process

This discussion is following the Openverse decision-making process. Information about this process can be found on the Openverse documentation site. Requested reviewers or participants will be following this process. If you are being asked to give input on a specific detail, you do not need to familiarise yourself with the process and follow it.

Current round

This discussion is currently in the Decision round. The deadline for this round is Friday, May 3rd.

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

@github-actions github-actions bot added the 🧱 stack: documentation Related to Sphinx documentation label Mar 25, 2024
@openverse-bot openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 📄 aspect: text Concerns the textual material in the repository labels Mar 25, 2024
Copy link

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

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.

New files ➕:

@fcoveram
Copy link
Contributor

Do you have a deadline in mind for this document @zackkrida?

If helpful, I found the following Figma documentation/post that could ease moving the design to frontend.

@zackkrida
Copy link
Member Author

@fcoveram I'm going to finish this today. I will look at these resources. Thanks!

@zackkrida zackkrida added the 🧭 project: implementation plan An implementation plan for a project label Apr 17, 2024
@zackkrida zackkrida marked this pull request as ready for review April 17, 2024 16:22
@zackkrida zackkrida requested a review from a team as a code owner April 17, 2024 16:22
@zackkrida zackkrida requested review from AetherUnbound, obulat and sarayourfriend and removed request for AetherUnbound April 17, 2024 16:22
Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

Looking great so far! I think there are some potential complications with the selector that need to be ironed out, and some other minor clarifications. I don't anticipate any blockers during the decision phase.

Here are pretend, simplified examples of this setup.

In our primary CSS file, we setup two lists of CSS variables with different
values when the `.dark-mode` class is present:
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make Tailwind respect this for the dark: prefix, we'll need to update the config for manual toggling, right? https://tailwindcss.com/docs/dark-mode#toggling-dark-mode-manually

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally forgot to document this, yes. I'll update, and make sure the documentation is clear that this is only relevant for the "escape hatch" of dark: and not the core color changing mechanism.

@zackkrida
Copy link
Member Author

zackkrida commented Apr 23, 2024

@sarayourfriend and @obulat any objection to me drafting the PR and moving on to the following updates?

  • Update the old snapshot names to include the "light" theme
  • Move verification and testing from production to staging
  • Update the implementation to default to the user's system preference as discussed here. Choosing "dark" will set a .dark-mode class, choosing "light" will set a light-mode class, and choosing "system" (the default value) will do nothing.
  • Update the tailwind config so that darkMode is set like this:
    /** @type {import('tailwindcss').Config} */
    module.exports = {
    darkMode: ['variant', [
      '@media (prefers-color-scheme: dark) { &:not(.light-mode *) }',
      '&:is(.dark-mode *)', // :is is so the specificity matches and there's not unexpected bahavior 
    ]],
    // ...
    }
    This makes sure that we default to the user preference but that the user preference is overwritten if they have chosen light or dark mode in the UI toggle.
  • Add a chart(s) for the different color scenarios (requested here)

@sarayourfriend
Copy link
Collaborator

Yes, sounds good to me.

@zackkrida zackkrida marked this pull request as draft April 24, 2024 11:54
@zackkrida zackkrida marked this pull request as ready for review April 24, 2024 11:54
@zackkrida zackkrida marked this pull request as draft April 24, 2024 11:54
@zackkrida zackkrida marked this pull request as ready for review April 30, 2024 18:37
@zackkrida
Copy link
Member Author

@obulat and @sarayourfriend this proposal has been updated, and is ready for final review in the Decision round.

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +263 to +265
2. Update our Cloudflare static page caching rule for the frontend in with
a Cookie bypass rule (in pseudocode, something like `and not
http.cookie contains "openverse_color_scheme"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish there were a way to cache each colour scheme, rather than bypassing all caching entirely. I think Cloudflare actually might support something like this with custom cache keys, but those are only for Cloudflare enterprise customers: https://developers.cloudflare.com/cache/how-to/cache-keys/#availability

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this make all of the users who select the dark mode bypass cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

@obulat yes, it would, but crucially only on the pages which are being cached: the homepage and all static pages. Also, due to SPA routing these, static pages are only served from the cache on the user's initial page load. Therefore, cached homepage and static page visits mark an overall small percentage of Openverse traffic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wanted to add more 'flavour' to the issue, but even if we were worried about the caching issue, we could solve it by caching and segmenting the cache at the nginx level (or replace it with varnish, something more suitable for complex caching rules). We'd already be doing that if we moved those routes "statically" out of the app or cached them at all, and we could segment on as many relevant cookies as needed (hopefully not too many!).

Just say, there are solutions to this if it ever became a problem.

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.

LGTM!
Thank you for mentioning the cache rules, too.

Comment on lines +263 to +265
2. Update our Cloudflare static page caching rule for the frontend in with
a Cookie bypass rule (in pseudocode, something like `and not
http.cookie contains "openverse_color_scheme"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this make all of the users who select the dark mode bypass cache?

Co-authored-by: Olga Bulat <[email protected]>
Co-authored-by: sarayourfriend <[email protected]>
@zackkrida zackkrida merged commit 82d81fb into main May 1, 2024
41 checks passed
@zackkrida zackkrida deleted the dark-mode-ip branch May 1, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 aspect: text Concerns the textual material in the repository 🌟 goal: addition Addition of new feature 🟨 priority: medium Not blocking but should be addressed soon 🧭 project: implementation plan An implementation plan for a project 🧱 stack: documentation Related to Sphinx documentation
Projects
Status: Accepted
Archived in project
Development

Successfully merging this pull request may close these issues.

Implementation Plan: Dark Mode Frontend
5 participants