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

removes the old default aria-checked value from properties table #1069

Merged
merged 5 commits into from
Oct 21, 2019
Merged

removes the old default aria-checked value from properties table #1069

merged 5 commits into from
Oct 21, 2019

Conversation

scottaohara
Copy link
Member

@scottaohara scottaohara commented Sep 28, 2019 •
edited by pr-preview bot
Loading

closes #826

removes the previous default state of undefined for the default state of “false” from the properties table, fully closing this issue.

Preview | Diff

@scottaohara
removes the old default value from properties table
40c308f 
closes #826

removes the previous default state of undefined for the default state of “false”.
@jnurthen
Copy link
Member

"Implicit Value for Role" needs removing on the Checkbox role too

@scottaohara
Copy link
Member Author

Ah will get on that. Thanks

@jnurthen
Copy link
Member

Ah will get on that. Thanks

Not right now :)

@scottaohara scottaohara changed the title removes the old default value from properties table removes the old default aria-checked value from properties table Sep 28, 2019
joanmarie
joanmarie requested changes Oct 2, 2019
View reviewed changes
index.html Show resolved Hide resolved
@scottaohara
re-adds table row
a3d3029 
add the implicit value for role row back in, but leave the implicit values cell empty.
@joanmarie joanmarie self-requested a review October 2, 2019 16:50
mcking65
mcking65 requested changes Oct 10, 2019
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.

Unless I am misunderstanding some aspect of this, I think we need undefined for some roles.

index.html Show resolved Hide resolved
jnurthen
jnurthen reviewed Oct 16, 2019
View reviewed changes
index.html Outdated Show resolved Hide resolved
jnurthen
jnurthen requested changes Oct 16, 2019
View reviewed changes
Copy link
Member

@jnurthen jnurthen left a comment

Choose a reason for hiding this comment

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

Default needs to be restored to undefined

mcking65
mcking65 approved these changes Oct 20, 2019
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.

LGTM; this is the change we need.

@jnurthen jnurthen merged commit 4a0955e into master Oct 21, 2019
jnurthen pushed a commit that referenced this pull request Oct 21, 2019
@scottaohara @jnurthen
removes the old default aria-checked value from checkbox role (#1069)
c67a1dd
@scottaohara scottaohara deleted the so_826 branch October 21, 2019 17:16
carmacleod pushed a commit that referenced this pull request May 7, 2020
@scottaohara @carmacleod
removes the old default aria-checked value from checkbox role (#1069)
3c76f7f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnurthen jnurthen jnurthen approved these changes

@joanmarie joanmarie joanmarie approved these changes

@mcking65 mcking65 mcking65 approved these changes

Assignees
No one assigned
Labels
Agenda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Checkbox has aria-checked required, but there is a default value for aria-checked
4 participants
@scottaohara @jnurthen @joanmarie @mcking65