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.

Sign up for GitHub

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: toggle all flavor selectors sticky on click #49526

Merged
merged 8 commits into from
Sep 9, 2023
Merged

doc: toggle all flavor selectors sticky on click #49526

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bcb1e61
doc: toggle all flavor selectors sticky on click
eldoy Sep 7, 2023
61d6008
Update doc/api_assets/api.js
eldoy Sep 7, 2023
3e91b7c
fix initial setup of stored state
eldoy Sep 7, 2023
7922161
use localStorage instead of sessionStorage
eldoy Sep 7, 2023
6ac5fc2
optimize check in loop
eldoy Sep 7, 2023
95e6ae6
remove empty local storage
eldoy Sep 7, 2023
3e7d654
Merge branch 'main' into doc/toggle-all-selectors
eldoy Sep 8, 2023
ad92cfc
rename select to toggle
eldoy Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions doc/api_assets/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,30 @@
updateHashes();
}

function setupFlavorSelectors() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function setupFlavorSelectors() {
function setupFlavorToggles() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed merge and rename.

const kFlavorPreference = 'customFlavor';
const vCommonJS = 'cjs';
const flavorSetting = sessionStorage.getItem(kFlavorPreference);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, pushed a fix to remove value when false.

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

Choose a reason for hiding this comment

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

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


flavorSelectors.forEach((selector) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

selector.checked = flavorSetting !== vCommonJS;
selector.addEventListener('click', (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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) => {

Copy link
Contributor Author

@eldoy eldoy Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const checked = e.target.checked;

if (checked) {
sessionStorage.removeItem(kFlavorPreference);
} else {
sessionStorage.setItem(kFlavorPreference, vCommonJS);
}

flavorSelectors.forEach((el) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

el.checked = checked;
});
});
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
});
}

function setupCopyButton() {
const buttons = document.querySelectorAll('.copy-button');
buttons.forEach((button) => {
Expand Down Expand Up @@ -182,6 +206,8 @@
// Make link to other versions of the doc open to the same hash target (if it exists).
setupAltDocsLink();

setupFlavorSelectors();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setupFlavorSelectors();
setupFlavorToggles();


setupCopyButton();
}

Expand Down