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

Jump to bottom

Tri-State Checkbox Example: Restore Focus Appearance and Update Code Style #1801

Merged
merged 33 commits into from Oct 12, 2021
Merged

Tri-State Checkbox Example: Restore Focus Appearance and Update Code Style #1801

merged 33 commits into from Oct 12, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Mar 4, 2021 •
edited by mcking65

I was looking at the mixed checkbox example and I noticed that at some point the accessibility features related to visual focus styling had been removed. I have have updated the example and also added accessibility documentation on the visual focus and hover styling improvements for accessibility.

Preview Link

Changes:

  • Renamed example HTML file: checkbox-mixed.html.
  • Updated JS code to use current coding practices.
  • Changed from using event.keyCode to event.key in event handlers.
  • Restored visual focus styling features that were removed from the example.
  • Added documentation of accessibility features.
  • Updated example to use SVG graphics

Review checklist

@jongund
Copy link
Contributor Author

jongund commented Mar 4, 2021

@nschonni
I having the same problem I have had with other pull requests, in that it is giving me an error for something in the examples\index.html file. I have not touched the file, so I am not sure why it is giving me an error.

The examples\index.html file is a generated file, what is it being checked for in my pull requests that could make it fail?

How does this get fixed?

@nschonni
Copy link
Contributor

nschonni commented Mar 4, 2021

I think the lint-staged hook might not pick up the file rename. You can manually run npm run reference-tables to update the example file

@jongund
Copy link
Contributor Author

jongund commented Mar 4, 2021

@nschonni

That fixed it, thanks!

jesdaigle
jesdaigle requested changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@jesdaigle jesdaigle left a comment

Choose a reason for hiding this comment

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

Tests failed with 2 unhandled rejections in test/tests/menubar_menubar-navigation.js.

@jongund
Copy link
Contributor Author

jongund commented Apr 14, 2021

@jesdaigle

Tests failed with 2 unhandled rejections in test/tests/menubar_menubar-navigation.js.

I did not touch the navigation menubar code, so I am not sure why the navigation menubar example/tests would be affected by my changes. It looks to me all the regression test are passing now. Is there still an issue?

@jongund jongund changed the title Updated mixed checkbox example to restore visual keyboard focus styling feature Checkbox: Updated mixed checkbox example to restore visual keyboard focus styling feature May 7, 2021
@zcorpan zcorpan force-pushed the update-tri-state-checkbox-js branch from f6cc523 to c382f33 Compare May 10, 2021 22:01
@zcorpan
Copy link
Member

zcorpan commented May 10, 2021

test/tests/menubar_menubar-navigation.js appears to be failing on main, and indeed this PR doesn't touch that test.

I've rebased this on main and resolved the conflict. Looks good to me.

@zcorpan zcorpan requested a review from jesdaigle May 10, 2021 22:16
charmarkk
charmarkk approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@charmarkk charmarkk left a comment

Choose a reason for hiding this comment

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

Looks good with Firefox/Chrome and NVDA/JAWS latest. HCM Black and White look good, but with HCM Black and Chrome, it seems like the first checkbox (All Condiments) is not taking on the selected color in Windows settings - the bottom ones (Individual Condiments) do. So perhaps just one thing missing from the mixed checkbox. Not a blocker though, it's still white on black, and nothing is missing!

mcking65
mcking65 requested changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

@jongund, I corrected some of the obvious copy/paste issues in the accessibility features, e.g., changed mentions of radio to checkbox, but there is still some incorrect text.

examples/checkbox/checkbox-mixed.html Outdated Show resolved Hide resolved
examples/checkbox/checkbox-mixed.html Outdated Show resolved Hide resolved
examples/checkbox/checkbox-mixed.html Show resolved Hide resolved
@mcking65 mcking65 changed the title Checkbox: Updated mixed checkbox example to restore visual keyboard focus styling feature Tri-State Checkbox Example: Restore Focus Appearance and Update Code Style Oct 12, 2021
@mcking65 mcking65 added Code Quality Non-functional code changes to satisfy APG code style guidelines and linters Example Page Related to a page containing an example implementation of a pattern QA for APG Examples Related to improve the quality and the acceptance of APG examples labels Oct 12, 2021
@mcking65 mcking65 added this to Next Steps in Checkbox Patterns and Examples Development Project via automation Oct 12, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Oct 12, 2021
@mcking65 mcking65 moved this from Next Steps to In Progress in Checkbox Patterns and Examples Development Project Oct 12, 2021
@mcking65 mcking65 merged commit ee28eda into main Oct 12, 2021
Checkbox Patterns and Examples Development Project automation moved this from In Progress to Complete Oct 12, 2021
@mcking65 mcking65 deleted the update-tri-state-checkbox-js branch October 12, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcorpan zcorpan zcorpan approved these changes

@mcking65 mcking65 mcking65 approved these changes

@charmarkk charmarkk charmarkk approved these changes

@jesdaigle jesdaigle jesdaigle approved these changes

Assignees
No one assigned
Labels
Code Quality Non-functional code changes to satisfy APG code style guidelines and linters Example Page Related to a page containing an example implementation of a pattern QA for APG Examples Related to improve the quality and the acceptance of APG examples
Projects
Checkbox Patterns and Examples Develo...
  
Complete
Milestone
1.2 Release 1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jongund @nschonni @zcorpan @mcking65 @charmarkk @jesdaigle