doc: toggle all flavor selectors sticky on click #49526

eldoy · 2023-09-07T10:07:26Z

Fixes: #49508

Fixes: nodejs#49508

nodejs-github-bot · 2023-09-07T10:07:30Z

Review requested:

@nodejs/website

doc/api_assets/api.js

aduh95 · 2023-09-07T13:07:41Z

doc/api_assets/api.js

+    const flavorSelectors = document.querySelectorAll('.js-flavor-selector');
+
+    flavorSelectors.forEach((selector) => {
+      selector.checked = flavorSetting !== vCommonJS;
+      selector.addEventListener('click', (e) => {


I find it a bit confusing to call them "selector", because selector usually refers to a CSS selector.

Suggested change

const flavorSelectors = document.querySelectorAll('.js-flavor-selector');

flavorSelectors.forEach((selector) => {

selector.checked = flavorSetting !== vCommonJS;

selector.addEventListener('click', (e) => {

const flavorToggles = document.querySelectorAll('.js-flavor-selector');

flavorToggles.forEach((toggleElement) => {

toggleElement.checked = flavorSetting !== vCommonJS;

toggleElement.addEventListener('click', (e) => {

Elsewhere in the file it's referred to as "selector", see "flavorSelector" in the "setupCopyButton" function in the same file. Would maybe be even more confusing if it's not the same as in the other function. The class name also ends with "selector".

I tried to mimic the style of the other functions to increase coherency.

But commit your suggestion if you feel like it's better, it's fine by me.

I opened #49536, let's see what the others think

Co-authored-by: Antoine du Hamel <[email protected]>

ovflowd · 2023-09-07T15:29:08Z

doc/api_assets/api.js

@@ -136,6 +136,25 @@
    updateHashes();
  }

+  function setupFlavorSelectors() {
+    const kFlavorPreference = 'customFlavor';
+    const flavorSetting = sessionStorage.getItem(kFlavorPreference);


I do not see any value in using a sessionStorage here. As the lifecycle would be contained to the current session. A localStorage makes more sense here.

Pushed a new version with localStorage. This leaves some "garbage" values there though since it doesn't get removed, which may be more of an issue with localStorage than with sessionStorage. Adding a "removeItem" when the value is not "true" could fix that and keep it clean, but maybe not a big issue.

I assume that localStorage should sync with the user preference. Either true or false :)

Yes, but it's not really necessary to store the string "false" it can just be undefined aka be removed (removeItem) to avoid wasting localStorage space. But if it's fine for you, then it's fine for me, may just be nitpicking.

Yeah, sorry, what I meant is that we should track both states. And for me the absence of a value is more than enough.

But we should remove then the value when set to false.

Ok, pushed a fix to remove value when false.

ovflowd · 2023-09-07T15:30:05Z

doc/api_assets/api.js

+    const flavorSelectors = document.querySelectorAll('.js-flavor-selector');
+
+    flavorSelectors.forEach((selector) => {
+      selector.checked = flavorSetting === 'true'


This is unoptimized. You're running this same check for every checkbox, knowing that this check is only needed once.

Also you probably don't need to set the value of the checkbox if flavorSetting is different from true

Pushed optimized code. You need to set the checkbox value also when it's not true, leaving that unchanged.

aduh95 · 2023-09-08T15:05:01Z

doc/api_assets/api.js

@@ -136,6 +136,29 @@
    updateHashes();
  }

+  function setupFlavorSelectors() {


Suggested change

function setupFlavorSelectors() {

function setupFlavorToggles() {

This comment applies to all current suggestions:

If you want to change every mention of "select" to "toggle", then we also need to change all the CSS files and also all the existing functions in api.js and possibly more. There are also test cases that might be affected.

I think this renaming thing should go in a separate issue so we can land this feature. No need to spend time risking introducing other issues on something that is not really a problem imho.

I think I've done all the preparation work in #49536, do you think I've missed something? We can't land this PR as is because it's now inconsistent with main, but I think all you need to do is to merge my suggestions and this can land as soon as we have a green CI.

Ok, so you've committed the renaming directly onto main already?

The "setupCopyButton" function in doc/api_assets/api.js also uses the name "selector" everywhere, is that included? Not sure what else.

Pushed merge and rename.

aduh95 · 2023-09-08T15:05:22Z

doc/api_assets/api.js

+  function setupFlavorSelectors() {
+    const kFlavorPreference = 'customFlavor';
+    const flavorSetting = localStorage.getItem(kFlavorPreference) === 'true';
+    const flavorSelectors = document.querySelectorAll('.js-flavor-selector');


Suggested change

const flavorSelectors = document.querySelectorAll('.js-flavor-selector');

const flavorToggles = document.querySelectorAll('.js-flavor-toggle');

aduh95 · 2023-09-08T15:05:35Z

doc/api_assets/api.js

+    const flavorSetting = localStorage.getItem(kFlavorPreference) === 'true';
+    const flavorSelectors = document.querySelectorAll('.js-flavor-selector');
+
+    flavorSelectors.forEach((selector) => {


Suggested change

flavorSelectors.forEach((selector) => {

flavorToggles.forEach((toggle) => {

aduh95 · 2023-09-08T15:05:45Z

doc/api_assets/api.js

+      selector.checked = flavorSetting;
+      selector.addEventListener('change', (e) => {


Suggested change

selector.checked = flavorSetting;

selector.addEventListener('change', (e) => {

toggle.checked = flavorSetting;

toggle.addEventListener('change', (e) => {

aduh95 · 2023-09-08T15:06:02Z

doc/api_assets/api.js

+          localStorage.removeItem(kFlavorPreference);
+        }
+
+        flavorSelectors.forEach((el) => {


Suggested change

flavorSelectors.forEach((el) => {

flavorToggles.forEach((el) => {

aduh95 · 2023-09-08T15:06:16Z

doc/api_assets/api.js

@@ -182,6 +205,8 @@
    // Make link to other versions of the doc open to the same hash target (if it exists).
    setupAltDocsLink();

+    setupFlavorSelectors();


Suggested change

setupFlavorSelectors();

setupFlavorToggles();

aduh95 · 2023-09-09T22:07:15Z

Landed in b500037, thanks for the contribution!

eldoy · 2023-09-09T22:36:42Z

Cool, thanks for the feedback!

PR-URL: #49526 Fixes: #49508 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

PR-URL: nodejs#49526 Fixes: nodejs#49508 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

doc: toggle all flavor selectors sticky on click

bcb1e61

Fixes: nodejs#49508

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 7, 2023

aduh95 reviewed Sep 7, 2023

View reviewed changes

doc/api_assets/api.js Outdated Show resolved Hide resolved

aduh95 reviewed Sep 7, 2023

View reviewed changes

eldoy and others added 2 commits September 7, 2023 15:37

Update doc/api_assets/api.js

61d6008

Co-authored-by: Antoine du Hamel <[email protected]>

fix initial setup of stored state

3e91b7c

ovflowd reviewed Sep 7, 2023

View reviewed changes

eldoy added 3 commits September 7, 2023 18:03

use localStorage instead of sessionStorage

7922161

optimize check in loop

6ac5fc2

remove empty local storage

95e6ae6

aduh95 reviewed Sep 8, 2023

View reviewed changes

eldoy added 2 commits September 8, 2023 21:19

Merge branch 'main' into doc/toggle-all-selectors

3e7d654

rename select to toggle

ad92cfc

aduh95 approved these changes Sep 8, 2023

View reviewed changes

atlowChemi approved these changes Sep 9, 2023

View reviewed changes

aduh95 merged commit b500037 into nodejs:main Sep 9, 2023
14 checks passed

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023

doc: save user preference for JS flavor

53fb5ae

PR-URL: #49526 Fixes: #49508 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

This was referenced Sep 28, 2023

v20.7.1 proposal #49917

Closed

v20.8.0 proposal #49932

Merged

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023

doc: save user preference for JS flavor

1fb730a

PR-URL: nodejs#49526 Fixes: nodejs#49508 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: toggle all flavor selectors sticky on click #49526

doc: toggle all flavor selectors sticky on click #49526

eldoy commented Sep 7, 2023

nodejs-github-bot commented Sep 7, 2023

aduh95 Sep 7, 2023

eldoy Sep 7, 2023 •

edited

Loading

eldoy Sep 7, 2023

aduh95 Sep 7, 2023

ovflowd Sep 7, 2023

eldoy Sep 7, 2023

ovflowd Sep 7, 2023

eldoy Sep 7, 2023

ovflowd Sep 7, 2023

eldoy Sep 7, 2023

ovflowd Sep 7, 2023

eldoy Sep 7, 2023

aduh95 Sep 8, 2023

eldoy Sep 8, 2023

aduh95 Sep 8, 2023

eldoy Sep 8, 2023

eldoy Sep 8, 2023

aduh95 Sep 8, 2023

aduh95 Sep 8, 2023

aduh95 Sep 8, 2023

aduh95 Sep 8, 2023

aduh95 Sep 8, 2023

aduh95 commented Sep 9, 2023

eldoy commented Sep 9, 2023

	function setupFlavorSelectors() {
	function setupFlavorToggles() {

	const flavorSelectors = document.querySelectorAll('.js-flavor-selector');
	const flavorToggles = document.querySelectorAll('.js-flavor-toggle');

	flavorSelectors.forEach((selector) => {
	flavorToggles.forEach((toggle) => {

		selector.checked = flavorSetting;
		selector.addEventListener('change', (e) => {

	flavorSelectors.forEach((el) => {
	flavorToggles.forEach((el) => {

doc: toggle all flavor selectors sticky on click #49526

doc: toggle all flavor selectors sticky on click #49526

Conversation

eldoy commented Sep 7, 2023

nodejs-github-bot commented Sep 7, 2023

Choose a reason for hiding this comment

eldoy Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 commented Sep 9, 2023

eldoy commented Sep 9, 2023

eldoy Sep 7, 2023 •

edited

Loading