-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Love the clean up and the new tests 🎉
aria-controls="content-switcher-popover" | ||
/> | ||
</template> | ||
<VContentTypes | ||
id="content-switcher-popover" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh interesting! I missed this requirement.
I wonder if there's a good way to remove the need to duplicate this prop value and also add a warning that it should be included (similar to the aria-label
/aria-labelledby
prop) 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a sign of another a11y problem a good idea. To be honest, I've read about aria-controls
and aria-haspopup, and still don't understand whether it should be dialog or menu, and whether aria-controls is strictly necessary, @sarayourfriend .
I added it here because I couldn't find a better more accessible way of selecting the content switcher in e2e tests (the text changes, so I can't use it). And this might bea sign of another a11y problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also was confused about that but looking at Reakit, as I often do to check on ideas for how to implement some of these patterns correctly, it does include aria-controls
in the Disclosure
component which is used for Dialog as well in DialogDisclosure
. So I think you're right to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the changeContentType
helper. LGTM!
Fixes
Related #797 by @sarayourfriend
Description
While working on the #835 solution, I saw a bug when the filters were not reset correctly when changing the content type, so I added the e2e tests that would catch this problem. While at it, I also quickly refactored the e2e tests to extract some often-used functions, and enabled some tests that were set to skip before.
E2e tests will be very important when converting to Pinia.
Testing Instructions
Run
pnpm e2e:ci
. This should build the app and run e2e tests. Only 2 tests should be skipped, others should pass.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin