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.

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

Adds checkboxes with localStorage for checklist items #1025

Merged
merged 7 commits into from
Aug 9, 2020

Conversation

SaptakS
Copy link
Member

@SaptakS SaptakS commented Jul 26, 2020

Fixes #942

  • Adds checkboxes to each checklist item
  • Stores the checklist state in localstorage

Copy link
Member

@ericwbailey ericwbailey left a comment

Choose a reason for hiding this comment

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

This is a really great start. What I would like to do before merging is to move the checkbox out of the details element and place it before it.

The reason for this being that the checkbox cannot be navigated to, or interacted with via keyboard controls—the existing details element overrides it.

@SaptakS
Copy link
Member Author

SaptakS commented Jul 28, 2020

The reason for this being that the checkbox cannot be navigated to, or interacted with via keyboard controls—the existing details element overrides it.

Aah. Makes sense.

@SaptakS
Copy link
Member Author

SaptakS commented Jul 28, 2020

So as suggested I have moved the checkbox and out of the details element. It should be navigable with keyboard.
Though I have few things that are bothering me with the design changes that came along with it. To keep the same design as before, I have made the checkbox float right (to appear on right) but via keyboard tab, it will be focused before the details. Secondly, when the summary is in focus, because of the blue color, the checkbox isn't visible. Now since the checkbox appears before the details, there is no way to style it when the summary is focused.

I am not 100% sure what the best way would be or in case you would want some design modifications based on these changes.

@ericwbailey
Copy link
Member

That's good to know. I'm a little busy until the weekend, but would you like to asynchronously pair on it? I figure I could take a shot at some of the design-related issues.

@SaptakS
Copy link
Member Author

SaptakS commented Jul 29, 2020

That's good to know. I'm a little busy until the weekend, but would you like to asynchronously pair on it? I figure I could take a shot at some of the design-related issues.

Sure!! Is there a channel somewhere to communicate, or do you prefer GitHub comments?

@ericwbailey
Copy link
Member

Great! I think GitHub comments would work. I'll tag you either here on in the review code for the parts I shift around.

@ericwbailey
Copy link
Member

@SaptakS This weekend turned out to be a little busier than expected. Hoping to get something up for you to check out sooner than later.

@SaptakS
Copy link
Member Author

SaptakS commented Aug 4, 2020

@ericwbailey no problem. :)

@ericwbailey
Copy link
Member

@SaptakS I just pushed up my changes, but I think it was accidentally to the checkbox-checklist branch on this repo, and not yours.

This is admittedly something I don't do that often, so I'll ask a friend for some help getting my changes incorporated into your repo tomorrow.

As for changes, here's the high level notes:

  • Created a new checkboxId entry for each item in the JSON, as the for/id pairing was breaking on account of slugify not processing some special entities we use.
  • Removed the SVG checkmark in favor of a rotated border, for more robust support.
  • Slight tweaks to the checkbox's accessible name.

@SaptakS
Copy link
Member Author

SaptakS commented Aug 9, 2020

@SaptakS I just pushed up my changes, but I think it was accidentally to the checkbox-checklist branch on this repo, and not yours.

I can actually fix this. The only thing is there are quite some merge conflicts (in both my commits and yours) with main. So I am trying to rebase properly. I will then try to push if everything looks okay

@SaptakS SaptakS force-pushed the checkbox-checklist branch from f02b550 to 9ad1c26 Compare August 9, 2020 20:16
@SaptakS
Copy link
Member Author

SaptakS commented Aug 9, 2020

@ericwbailey okay. I have rebased on both main and your branch to include all your changes in this PR. So it should be good to go for review.

@SaptakS
Copy link
Member Author

SaptakS commented Aug 9, 2020

* Removed the SVG checkmark in favor of a rotated border, for [more robust support](https://adrianroselli.com/2017/05/under-engineered-custom-radio-buttons-and-checkboxen.html).

Also thanks for this link. This is awesome!

@ericwbailey
Copy link
Member

@SaptakS Absolutely amazing. Thank you so much 🎉

@ericwbailey ericwbailey merged commit b7754fc into a11yproject:main Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checkboxes to checklist items
2 participants