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

fix: Allow divs as groups in dl #1076

Merged
merged 2 commits into from
Aug 22, 2018
Merged

fix: Allow divs as groups in dl #1076

merged 2 commits into from
Aug 22, 2018

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Aug 15, 2018 •
edited by jeeyyy
Loading

Allow div elements to group content inside of dl elements.

Closes #262

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: Jey

@WilcoFiers WilcoFiers requested a review from a team as a code owner August 15, 2018 13:58
@kr8n3r kr8n3r mentioned this pull request Aug 21, 2018
Open
jeeyyy
jeeyyy approved these changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@jeeyyy jeeyyy left a comment

Choose a reason for hiding this comment

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

Lgtm.

dylanb
dylanb approved these changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

If you wanted to make the tests a bit more robust, you could add a test that makes sure a div with children that are not ALL dl/dt or where NONE are dl/dt fail

test/checks/lists/only-dlitems.js
);
});

it('should return true if the list has dt and dd inside a div group with a role', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a test with other content inside the div?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolving in a new PR

@WilcoFiers WilcoFiers merged commit f4f6df6 into develop Aug 22, 2018
@WilcoFiers WilcoFiers deleted the dl-divs branch August 22, 2018 09:17
@WilcoFiers WilcoFiers mentioned this pull request Aug 22, 2018
Merged
4 tasks
@marcysutton marcysutton mentioned this pull request Oct 24, 2018
Closed
Malvoz added a commit to Malvoz/axe-core that referenced this pull request Mar 13, 2020
@Malvoz
Mention <div> as allowed element for <dt>/<dd> groups
e1632d0 
Ref: dequelabs#1076
Malvoz added a commit to Malvoz/axe-core that referenced this pull request Mar 13, 2020
@Malvoz
docs: mention <div> allowed element for <dd>/<dt> groups
e3d6dab 
Ref: dequelabs#1076.
@Malvoz Malvoz mentioned this pull request Mar 13, 2020
Merged
2 tasks
Malvoz added a commit to Malvoz/lighthouse that referenced this pull request Mar 13, 2020
@Malvoz
docs: <div> is allowed to be used for grouping content in <dl>
6017b5d 
axe ref: dequelabs/axe-core#1076.
@Malvoz Malvoz mentioned this pull request Mar 18, 2020
Merged
straker pushed a commit that referenced this pull request Mar 31, 2020
@Malvoz @straker
docs: mention <div> allowed element for <dd>/<dt> groups
770d05d 
Ref: #1076.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeeyyy jeeyyy jeeyyy approved these changes

@dylanb dylanb dylanb approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@WilcoFiers @dylanb @jeeyyy