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

docs: add dark-mode capability #2896

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

meysam81
Copy link

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Having the ability to choose between dark and light mode is crucial when reading documents online behind LED lights. This PR is an effort to make that a reality for people who prefer reading in dark mode, especially myself.

Which issue(s) this PR fixes:

N/A

Does this PR introduce a user-facing change?:

Just the docs.

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 23, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 23, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @meysam81!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 23, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @meysam81. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 23, 2024
@youngnick
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 28, 2024
@youngnick
Copy link
Contributor

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 28, 2024
@youngnick
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: meysam81, youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2024
@meysam81
Copy link
Author

meysam81 commented Apr 6, 2024

ping @youngnick @shaneutt

@robscott
Copy link
Member

robscott commented Apr 9, 2024

Hey @meysam81, thanks for adding this! Do you mind changing the default to still be light mode? We still have a variety of graphics that haven't been adapted to work well on dark mode, so I don't think we're ready to use that as a default in any case, but I think it's fine if people want to manually opt in.

@meysam81
Copy link
Author

meysam81 commented Apr 9, 2024

@robscott
Without any further change, the current default is the system default, which is more adaptable to the user's settings. Are you suggesting that it's better to have the light mode as default altogether? 🤔

@robscott
Copy link
Member

robscott commented Apr 9, 2024

@robscott Without any further change, the current default is the system default, which is more adaptable to the user's settings. Are you suggesting that it's better to have the light mode as default altogether? 🤔

Yep, until we can update the graphics to adjust for dark mode, I think we need to default to the light mode. I default to dark mode on my computers but would still personally prefer the light mode on the website until we can also make corresponding updates to our graphics. For example, just looking at the main page, both the diagram and logo do not look ideal with dark mode. I think it's great to offer the choice, I just wouldn't want dark mode to be default for anyone quite yet.
Screenshot 2024-04-08 at 8 22 50 PM

@meysam81
Copy link
Author

meysam81 commented Apr 9, 2024

@robscott
Yes, having the option to choose the dark mode is great, and I don't mind having to do it manually once (and not touching the cookies ever 😁 ). For me, I would rather see less appealing graphics than having to endure light mode; it gets more frustrating after evenings.
But I'll push for the change shortly.
Thanks for taking the time to review this.

@meysam81
Copy link
Author

kindly take a look 🙏
@youngnick @shaneutt @robscott

@robscott
Copy link
Member

@meysam81 I may be missing something, but at least for me on Chrome, it's still defaulting to dark mode (matching my system preferences).

@meysam81
Copy link
Author

@robscott

Would you try again, please? This last change should do it.

@robscott
Copy link
Member

Hey @meysam81, sadly still seeing the same results on Chrome and Safari where it defaults to dark mode for me (matching my OS).

@meysam81
Copy link
Author

@robscott Does it by any chance have a different outcome when clearing cookies and/or trying from incognito I wonder?

@robscott
Copy link
Member

@robscott Does it by any chance have a different outcome when clearing cookies and/or trying from incognito I wonder?

Yep, incognito and private modes respectively.

@meysam81
Copy link
Author

@robscott

The behavior of the theme following the system preferences seems to be a design decision, as explained here: squidfunk/mkdocs-material#7093 (reply in thread)

It seems like this PR can be accepted or rejected as is because, as far as my understanding goes, there can't be a configuration change that can force the light mode on a system with a dark-mode preference. 🤷

@robscott
Copy link
Member

Sorry for the delay on this @meysam81! We discussed this at a community meeting and the overall consensus was that we should hold off until we can also update the images. I've created #3146 to track that effort.

@shaneutt
Copy link
Member

Blocked on #3146 then it seems:

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2024
@k8s-ci-robot
Copy link
Contributor

@meysam81: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-gateway-api-crds-validation-4 8bfa4d5 link true /test pull-gateway-api-crds-validation-4

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@sftim
Copy link

sftim commented Nov 22, 2024

Folks, I recommend aligning with the Kubernetes website work on this - see kubernetes/website#45535

Obviously we don't have to align, but our users are likely to prefer consistent over inconsistent user experience.

@sftim
Copy link

sftim commented Nov 22, 2024

It seems like this PR can be accepted or rejected as is because, as far as my understanding goes, there can't be a configuration change that can force the light mode on a system with a dark-mode preference. 🤷

You can use CSS to do something close

:root {
  color-scheme: only light; // dark mode support is incomplete
}

and then optionally let people opt in to something that overrides that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/documentation Categorizes issue or PR as related to documentation. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants