Feat/toggle checklist groups #1280

rachel-fischoff · 2021-05-21T21:08:02Z

closes #940

Summary

Creates a Toggle Button for each section of the Checklist (toggle content, toggle keyboard, etc) that allows you to mass open or close all of the details for each section. Additoinally, a toggle all button is created to expand and collapse all the checklist group items on the entire Checklist page.

Images

Below is the Toggle Content before being clicked

Below is the Toggle Content after being clicked and expanded

Below is the Toggle All button located above the first checklist group

Next Steps

Would love to get some input on the styling especially for the Toggle All button!

… file

…licked

boring-cyborg · 2021-05-21T21:08:05Z

Thank you for opening this Pull Request! Please make sure you've read our Code of Conduct and Contributing guidelines.

ericwbailey

This looks good to me! Thank you for submitting it, @rachel-fischoff.

@SaptakS: Could I get a second set of eyes on the JS?

mxmason

Accidentally created this review without code comments. Please refer to #1280 (review)

mxmason · 2021-06-07T19:20:59Z

src/js/checklist.js

+
+renderToggleAll();
+
+function renderToggleCategories(){


🔍 You can probably write a query selector that gets both #toggle-all and all the other buttons, which then means you won't need a dedicated function for renderToggleAll.

src/js/checklist.js

mxmason

Thanks for writing this PR, @rachel-fischoff, and thank you for your patience with our reviews! Your code looks great! I would like to take care of some things that were not in #940, but which I consider to be blocking.

Issues

These new toggle buttons are not very helpful without JavaScript, so IMO they should be hidden initially, and then shown once we can confirm that JavaScript is on the page.
These new buttons need styling in general, so they look like they fit in with our existing design and layout.
IMO, if the checklist items in a category are not all in the same open state, we should either open all of them or close all of them.

Expanding (ha) issue number 3:

Say the user opens "Use plain language..." to expand it so only that item is open while all the others are closed. Then the user clicks "Toggle Content".

Current behavior: All of the items in the category flip states. "Use plain language..." is closed; all the others open
Desired behavior: All of the items in the category are opened or closed.

Next steps

Decide how we ship this. Do we improve the style of the buttons, ship the feature, then come back and improve the open/close state management, or do we do all of this in one PR?
Decide what to do in the case of mixed states.
- In the case of mixed states, like above, what should the next state be – open, or closed?
- How will we handle the Toggle all button if one or more items in any category is open?

Thoughts, @ericwbailey and @rachel-fischoff?

ericwbailey · 2021-06-16T23:43:33Z

Hi @rachel-fischoff!

First off I want to apologize for the delay in responding to this. I've been going through a ton of work-related things and it's taken a lot of time I'd normally have for work here.

This is going to be a very welcome addition to the site, and I'm really happy you did the work. For my perspective on @mxmason's questions:

Decide how we ship this. Do we improve the style of the buttons, ship the feature, then come back and improve the open/close state management, or do we do all of this in one PR?

For styling, I'm very familiar with this site's Sass, so I was figuring I could create a followup styling PR once the logic is done/

As for the logic, I think we should get it into the desired state in this PR.

Decide what to do in the case of mixed states.

My thinking here is:

Toggle all:

If pressed, all checklist items convert to open state, regardless of their category or prior state.
If pressed again, checklist items revert to a closed state.

Toggle section:

If pressed, all checklist items in the relevant section convert to open state, regardless of their category or prior state.
If pressed again, checklist items update to a closed state.

I am modeling my thinking here after similar OS UI behavior in terms of external consistency (I'm thinking file trees and similar kinds of interactive nested list content).

The other thing that I think is tangentially relevant is toggle states and accessible names. I don't think this is relevant here directly, in that the existing accessible name plus the ARIA we're using should be sufficient?

rachel-fischoff · 2021-06-16T23:52:37Z

Thank you @mxmason & @ericwbailey for this very thorough feedback! I will get to work on the changes early next week! I'll check in if I have any additional questions!

ericwbailey · 2021-06-16T23:56:13Z

Yes! Really looking forward to it 🙂

…gories to one function

…hangeAttribute

…eButtonClick'

…ts when clicked

rachel-fischoff · 2021-08-26T01:21:30Z

@mxmason & @ericwbailey Hey y'all! I finally finished the functionality! I'm ready for styling help. Thanks for your patience. Let me know if you have questions or comments.

SaptakS

Works great! Added few knitpicking comments. There also seems to be some merge conflict issues that got into the package-lock.json file. Apart from that, I guess @ericwbailey we need some designs to match the rest of the style of the website.

package-lock.json

src/js/checklist.js

src/checklist.njk

rachel-fischoff · 2023-04-10T04:36:21Z

I know this is almost 2 years old but should I close this PR @mxmason @ericwbailey or should I still try to get this functionality merged in? Not sure if it's even desired anymore.

SaptakS · 2023-04-11T16:54:10Z

I think @ericwbailey might be able to comment better, but I would definitely love to see this PR merged given you still have the capacity. I can do a re-review to see if all the above concerns are addressed and what is left if that helps. I think @ericwbailey said he can make a commit for the design separately once functionality is done. In case he is not available, I can use existing designs in the sass code to make it look similar to the rest of the website.

rachel-fischoff · 2023-04-11T22:31:41Z

@SaptakS I think I had addressed all the comments beforehand and was hading to hear back. If I recall, I was just waiting on the design but let me know about functionality. I can also update with the latest main.

SaptakS · 2023-04-12T06:54:58Z

I will take a look.

SaptakS

The code seems to be not functioning right now. I left a few comments which I think should help resolve that. Also, @mxmason mentioned that we should hide the toggle buttons by default and only show them from the javascript code, because the toggle buttons don't work without javascript, and hence shouldn't be visible for non-js users.

SaptakS · 2023-04-12T07:38:51Z

src/js/checklist.js

+	registerToggleButton(toggleAllButton, document);
+
+	var toggleCategoryButtons = document.querySelectorAll(
+		"[data-toggle-category]"


I think this selector is not working anymore because the njk code no longer has this data attribute. Probably can use .toggle-category instead? I think because of this the category toggles are not working anymore.

SaptakS · 2023-04-12T08:10:58Z

src/js/checklist.js

+		buttonEl.addEventListener("click", function handleToggleButtonClick(event) {
+			var target = event.target;
+			if (target.hasAttribute("data-open") === true) {
+				target.removeAttribute("data-open");


Can we instead use aria-expanded="true" in place of data-open? I think that will help convey meaningful information to the assistive technology about the state of the button as well, and helps with the logic as well.
For reference: https://adrianroselli.com/2020/05/disclosure-widgets.html#Script

SaptakS · 2023-04-12T08:11:04Z

src/js/checklist.js

+
+function registerToggleButton(buttonEl, parentEl) {
+	if (buttonEl && parentEl) {
+		var details = parentEl.querySelectorAll("details");


Can we make this selector a little more specific? Like details.c-checklist to ensure that we are not toggle any other <details> that might be there. Just to future-proof it a bit.

rachel-fischoff · 2023-04-12T13:47:59Z

@SaptakS will address this weekend and get back to you!

rachel-fischoff · 2023-04-30T00:10:16Z

@SaptakS Sorry, I've been sick but I'll message you when I have the fix.

SaptakS · 2023-04-30T03:49:10Z

@rachel-fischoff no need of apologies and no hurries. Take your time! And I hope you feel better soon.

rachel-fischoff · 2024-01-16T22:41:32Z

Hi @SaptakS and team. I keep overestimating my capacity and haven't been in JavaScript land in a long time. I think it makes the most sense to just close this PR and then comment on the issue that I never got around to it. If someone wants to open it back up or continue this work then they are more than welcome.

SaptakS · 2024-01-17T18:47:40Z

That's totally fine! I don't think this PR needs to be closed. Especially since it doesn't have any merge conflicts. I think anyone (or one of us) who wants to work on the issue can continue on this PR. I haven't checked however if this branch can be pushed into by us. If not, I might create a new PR with your commits and continue from there. But I don't want to close this PR and make your work go away. :) Thanks for your contributions!

rachel-fischoff added 9 commits May 6, 2021 18:04

toogle-checklist button created + next steps outlined in checklist.js…

34aee14

… file

toggle content button created

0b401bf

toggle content button created

3762540

toggle content button created

08f6a47

updated feat/toggle-checklist-groups with main

1e4c7e3

toggle content button renders summary for details in content group

6945378

feat: toggle all & toggle category opening and closing details when c…

529ecdc

…licked

feat: refactor create, created reusable function registerToggleButton

e6adce2

refactor: removed unncessary semicolons and warnings

511eef0

boring-cyborg bot added checklist Updates for the web accessibility checklist javascript Issues dealing with JavaScript. markup Issues dealing with markup labels May 21, 2021

ericwbailey approved these changes May 26, 2021

View reviewed changes

mxmason self-requested a review June 7, 2021 18:45

mxmason requested changes Jun 7, 2021

View reviewed changes

mxmason requested a review from ericwbailey June 7, 2021 18:57

mxmason reviewed Jun 7, 2021

View reviewed changes

src/js/checklist.js Outdated Show resolved Hide resolved

mxmason requested changes Jun 7, 2021

View reviewed changes

rachel-fischoff added 5 commits July 6, 2021 17:22

merged with current main

2f490a5

updated code with main & combined renderToggleAll & renderToggleCater…

f56fdf7

…gories to one function

changed function from anonymous event listener to a named function: c…

cb8dc8c

…hangeAttribute

refactored and added correct event handler function name 'handleToggl…

0c9a3c5

…eButtonClick'

fix: toggle category button & toggle all open or close all the elemen…

edb1f05

…ts when clicked

SaptakS requested changes Aug 26, 2021

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

src/js/checklist.js Outdated Show resolved Hide resolved

src/checklist.njk Outdated Show resolved Hide resolved

rachel-fischoff added 3 commits August 26, 2021 16:58

fix: removed git merge remanant in package-lock.json

d97747c

fix: removed details attribute open and changed to data-open

2ea8d08

fix: changed data attribute to class for button

e8915ba

Merge branch 'main' into feat/toggle-checklist-groups

f7eec09

SaptakS requested changes Apr 12, 2023

View reviewed changes

ericwbailey mentioned this pull request May 10, 2023

Stepping down as a Maintainer #1550

Open

20 tasks

SaptakS self-assigned this Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/toggle checklist groups #1280

Feat/toggle checklist groups #1280

rachel-fischoff commented May 21, 2021

boring-cyborg bot commented May 21, 2021

ericwbailey left a comment

mxmason left a comment •

edited

mxmason Jun 7, 2021

rachel-fischoff Aug 16, 2021

mxmason left a comment •

edited

ericwbailey commented Jun 16, 2021

rachel-fischoff commented Jun 16, 2021

ericwbailey commented Jun 16, 2021

rachel-fischoff commented Aug 26, 2021

SaptakS left a comment

rachel-fischoff commented Apr 10, 2023

SaptakS commented Apr 11, 2023

rachel-fischoff commented Apr 11, 2023 •

edited

SaptakS commented Apr 12, 2023

SaptakS left a comment

SaptakS Apr 12, 2023

SaptakS Apr 12, 2023

SaptakS Apr 12, 2023

rachel-fischoff commented Apr 12, 2023

rachel-fischoff commented Apr 30, 2023

SaptakS commented Apr 30, 2023

rachel-fischoff commented Jan 16, 2024

SaptakS commented Jan 17, 2024

Feat/toggle checklist groups #1280

Are you sure you want to change the base?

Feat/toggle checklist groups #1280

Conversation

rachel-fischoff commented May 21, 2021

Summary

Images

Next Steps

boring-cyborg bot commented May 21, 2021

ericwbailey left a comment

Choose a reason for hiding this comment

mxmason left a comment • edited

Choose a reason for hiding this comment

mxmason Jun 7, 2021

Choose a reason for hiding this comment

rachel-fischoff Aug 16, 2021

Choose a reason for hiding this comment

mxmason left a comment • edited

Choose a reason for hiding this comment

Issues

Expanding (ha) issue number 3:

Next steps

ericwbailey commented Jun 16, 2021

rachel-fischoff commented Jun 16, 2021

ericwbailey commented Jun 16, 2021

rachel-fischoff commented Aug 26, 2021

SaptakS left a comment

Choose a reason for hiding this comment

rachel-fischoff commented Apr 10, 2023

SaptakS commented Apr 11, 2023

rachel-fischoff commented Apr 11, 2023 • edited

SaptakS commented Apr 12, 2023

SaptakS left a comment

Choose a reason for hiding this comment

SaptakS Apr 12, 2023

Choose a reason for hiding this comment

SaptakS Apr 12, 2023

Choose a reason for hiding this comment

SaptakS Apr 12, 2023

Choose a reason for hiding this comment

rachel-fischoff commented Apr 12, 2023

rachel-fischoff commented Apr 30, 2023

SaptakS commented Apr 30, 2023

rachel-fischoff commented Jan 16, 2024

SaptakS commented Jan 17, 2024

mxmason left a comment •

edited

mxmason left a comment •

edited

rachel-fischoff commented Apr 11, 2023 •

edited