Skip to content

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-botnodejs-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
Copy link
Member

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

@demakoffdemakoffforce-pushed the align-themes branch 2 times, most recently from 0292266 to c95d9bbCompareNovember 24, 2023 13:33
@ovflowd
Copy link
Member

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

@ovflowdovflowd added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2023
@nodejs-github-botnodejs-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 [email protected]
Reviewed-By: Yagiz Nizipli [email protected]

[detached HEAD 5fd00cecbb] doc: make theme consistent across api and other docs
Author: Dima Demakov [email protected]
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 [email protected]
Reviewed-By: Yagiz Nizipli [email protected]

[detached HEAD 0de5adbd5f] doc: update formatting
Author: Dima Demakov [email protected]
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 [email protected]
Reviewed-By: Yagiz Nizipli [email protected]

[detached HEAD 4d1a613278] Update md formatting
Author: Dima Demakov [email protected]
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-botnodejs-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

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

@demakoff
Copy link
ContributorAuthor

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.

@ovflowdovflowd 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-botnodejs-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.

@ovflowdovflowd 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-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2023
@nodejs-github-botnodejs-github-bot merged commit 62fc950 into nodejs:mainNov 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 62fc950

@demakoffdemakoff 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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
@RafaelGSSRafaelGSS 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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
@UlisesGasconUlisesGascon 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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
aduh95 pushed a commit that referenced this pull request Feb 10, 2025
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@demakoff@nodejs-github-bot@ovflowd@RaisinTen@bmuenzenmeyer@anonrig@aduh95