search

prateekbh / preact-material-components

preact wrapper for "Material Components for the web"
https://material.preactjs.com
MIT License
553 stars 81 forks source link

Dialog: change to dialog element #1289

Closed kontrollanten closed 5 years ago

kontrollanten commented 5 years ago

According to https://github.com/dequelabs/axe-core/blame/develop/lib/commons/aria/index.js#L279 an alertdialog should be of element type dialog or section.

Closes #1288

cromefire commented 5 years ago

I'm not sure that this is the right place, maybe it suited better at mdc, they specify the html

kontrollanten commented 5 years ago

In their docs they're using div element (which is also wrong, but not aside), so this shouldn't be aligned with mds? https://github.com/material-components/material-components-web/tree/master/packages/mdc-dialog

cromefire commented 5 years ago

Well I guess the dialog doesn't break anything right? The only problem it that you should also create a PR against 2.0 because we already split them off.

prateekbh commented 5 years ago

ummm doesn't dialog have its own functionality for showing a modal?

kontrollanten commented 5 years ago

Yes, it have. My bad. Shall we use section instead?

cromefire commented 5 years ago

Hmm I didn't know that dialog had that ability, I thought it's just some made up tag

prateekbh commented 5 years ago

Why not div with proper aria?

I know mdc web takes care of accessibility so I don't think it's s problem

kontrollanten commented 5 years ago

According to https://github.com/dequelabs/axe-core/blame/develop/lib/commons/aria/index.js#L279 it's not allowed, which gives me problem in my nightwatch-accesibility tests (built upon axe-core).

prateekbh commented 5 years ago

Hmm then may section of the second best option.