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 6 commits
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
25 changes: 25 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,29 @@
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 flavorSetting = localStorage.getItem(kFlavorPreference) === 'true';
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;
selector.addEventListener('change', (e) => {
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
selector.checked = flavorSetting;
selector.addEventListener('change', (e) => {
toggle.checked = flavorSetting;
toggle.addEventListener('change', (e) => {

const checked = e.target.checked;

if (checked) {
localStorage.setItem(kFlavorPreference, true);
} else {
localStorage.removeItem(kFlavorPreference);
}

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;
});
});
});
}

function setupCopyButton() {
const buttons = document.querySelectorAll('.copy-button');
buttons.forEach((button) => {
Expand Down Expand Up @@ -182,6 +205,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