Conversation
simeonoff
added
a11y
igdmdimitrov
added
🛠️ status: in-development
and removed
❌ status: awaiting-test
labels
# Conflicts: # src/index.ts
rkaraivanov
left a comment
rkaraivanov
left a comment
There was a problem hiding this comment.
After some reading up, the first point in the article
caught my attention.
This begs the question what do we render when the dialog is opened in this scenario?
<igc-dialog title="Warning"></igc-dialog>
Maybe we should have a default button when no content is provided just to move focus there for easier closing?
|
@rkaraivanov Yes, dialogs must have at least one focusable control so I think it's a good idea to always have an OK button. |
rkaraivanov
requested review from
damyanpetev and
rkaraivanov
and removed request for
rkaraivanov
ChronosSF
requested review from
rkaraivanov
and removed request for
rkaraivanov
damyanpetev
left a comment
damyanpetev
left a comment
There was a problem hiding this comment.
Outside of a minor nit and a questionable test, LGTM
| expect(dialog).dom.to.have.text(content); | ||
| expect(dialog).dom.to.equal( | ||
| ` | ||
| <igc-dialog> | ||
| <span> | ||
| Dialog content | ||
| </span> | ||
| </igc-dialog>`, |
There was a problem hiding this comment.
Confused. No way this is actually testing if the dialog has the content since it's not looking in the shadow DOM - so not really checking if the content is inside the dialog for real.
Same line of thinking for the second check - which again tests the light DOM for the very same nodes that were used in createDialogComponent, thus testing if the fixture and native DOM node creation works, kinda pointless.
There was a problem hiding this comment.
PS: Tested by deleting the default slot in the dialog, this test still passed :)
| <div part="backdrop" aria-hidden="true" ?hidden=${!this.open}></div> | ||
| <dialog | ||
| part="base" | ||
| role="dialog" |
There was a problem hiding this comment.
that role should be implicit on the native dialog already, so perhaps a bit redundant
9 participants
Closes #175