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

doc: make theme consistent across api and other docs (from nodejs.org repo) #50877

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

demakoff
Copy link
Contributor

Problem: since the website is based on 2 different repos (this and nodejs.org), there was an inconsistency in theme selection, so we had 2 independent theme props (one in session storage and another in local storage). With these changes, only one stored in local storage (theme: light | dark) is a single source of truth across all pages.

Before:
theme_before

After:
theme_after

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/nodejs-website
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Nov 23, 2023
BUILDING.md Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

The change looks good to me. Keep in mind we're eventually going to redesign the API Docs together with the Node.js Website.

doc/api_assets/api.js Outdated Show resolved Hide resolved
doc/api_assets/api.js Outdated Show resolved Hide resolved
doc/api_assets/api.js Outdated Show resolved Hide resolved
@demakoff demakoff force-pushed the align-themes branch 2 times, most recently from 0292266 to c95d9bb Compare November 24, 2023 13:33
@ovflowd
Copy link
Member

ovflowd commented Nov 24, 2023

@demakoff please run make format-md

Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth
@demakoff

This comment was marked as resolved.

@ovflowd

This comment was marked as resolved.

@demakoff

This comment was marked as resolved.

@ovflowd

This comment was marked as resolved.

@demakoff

This comment was marked as resolved.

@ovflowd ovflowd added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/50877
✔  Done loading data for nodejs/node/pull/50877
----------------------------------- PR info ------------------------------------
Title      doc: make theme consistent across api and other docs (from nodejs.org repo) (#50877)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     demakoff:align-themes -> nodejs:main
Labels     doc, build
Commits    3
 - doc: make theme consistent across api and other docs
 - doc: update formatting
 - Update md formatting
Committers 1
 - Dima Demakov 
PR-URL: https://github.com/nodejs/node/pull/50877
Reviewed-By: Claudio Wunder 
Reviewed-By: Yagiz Nizipli 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50877
Reviewed-By: Claudio Wunder 
Reviewed-By: Yagiz Nizipli 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 23 Nov 2023 12:01:04 GMT
   ✔  Approvals: 2
   ✔  - Claudio Wunder (@ovflowd): https://github.com/nodejs/node/pull/50877#pullrequestreview-1749420896
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/50877#pullrequestreview-1748014170
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 50877
From https://github.com/nodejs/node
 * branch                  refs/pull/50877/merge -> FETCH_HEAD
✔  Fetched commits as 4466deeb3432..56045316fd35
--------------------------------------------------------------------------------
[main e3efa97947] doc: make theme consistent across api and other docs
 Author: Dima Demakov 
 Date: Fri Nov 24 14:33:24 2023 +0100
 3 files changed, 15 insertions(+), 8 deletions(-)
[main 1a53995fdb] doc: update formatting
 Author: Dima Demakov 
 Date: Fri Nov 24 16:52:00 2023 +0100
 1 file changed, 1 insertion(+), 2 deletions(-)
[main e27f783a13] Update md formatting
 Author: Dima Demakov 
 Date: Sun Nov 26 18:01:56 2023 +0100
 1 file changed, 4 insertions(+), 3 deletions(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: make theme consistent across api and other docs

Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder cwunder@gnome.org
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 5fd00cecbb] doc: make theme consistent across api and other docs
Author: Dima Demakov d.demakov@gmail.com
Date: Fri Nov 24 14:33:24 2023 +0100
3 files changed, 15 insertions(+), 8 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: update formatting

PR-URL: #50877
Reviewed-By: Claudio Wunder cwunder@gnome.org
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 0de5adbd5f] doc: update formatting
Author: Dima Demakov d.demakov@gmail.com
Date: Fri Nov 24 16:52:00 2023 +0100
1 file changed, 1 insertion(+), 2 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update md formatting

PR-URL: #50877
Reviewed-By: Claudio Wunder cwunder@gnome.org
Reviewed-By: Yagiz Nizipli yagiz.nizipli@sentry.io

[detached HEAD 4d1a613278] Update md formatting
Author: Dima Demakov d.demakov@gmail.com
Date: Sun Nov 26 18:01:56 2023 +0100
1 file changed, 4 insertions(+), 3 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/6998992998

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 27, 2023
@ovflowd
Copy link
Member

ovflowd commented Nov 27, 2023

@demakoff I think we need an email (even if an anonymous one) to be listed on your GitHub profile

@demakoff
Copy link
Contributor Author

demakoff commented Nov 27, 2023

@demakoff I think we need an email (even if an anonymous one) to be listed on your GitHub profile

Super weird TBH. I have a few specified, incl the primary one which is configured in git to use on commit 🤔
Ok, I've specified that primary as public, hope it will help. Can we try to add in to the queue once again plz @ovflowd

@ovflowd

This comment was marked as resolved.

@ovflowd ovflowd added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 27, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 27, 2023
@nodejs-github-bot

This comment was marked as resolved.

@RaisinTen

This comment was marked as resolved.

@demakoff

This comment was marked as resolved.

@ovflowd

This comment was marked as resolved.

@ovflowd ovflowd added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 27, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit 62fc950 into nodejs:main Nov 27, 2023
31 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 62fc950

@demakoff demakoff deleted the align-themes branch November 27, 2023 10:15
RafaelGSS pushed a commit that referenced this pull request Nov 27, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
RafaelGSS pushed a commit that referenced this pull request Nov 30, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 13, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
Since website based on 2 different repos, there was an inconsistency
in theme selection, so we had 2 independant theme props.
Now only one stored in local storage is a single source of truth

PR-URL: #50877
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants