-
Notifications
You must be signed in to change notification settings - Fork 20
Remove aria-expanded attr from release announcement anchor #384
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.
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
Conversation
I'll leave the comment to do the explaining
Deploying compound-web with
|
| Latest commit: |
19ad3bf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://563b31c6.compound-web.pages.dev |
| Branch Preview URL: | https://dbkr-release-announcement-re.compound-web.pages.dev |
With an axe-exclude tags for stories that don't pass (which is a slightly frightening number...)
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.
This looks unexpected
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.
The reasoning here is that rather than having the anchor element control the visibility of the popup, we make it something else, that way the anchor element doesn't have to be a button, which I needed so I can add a test for when it's a div. I included the extra button so you could open and close the popup, although only because the functionality was there before: we could just have a prop to control whether it's open?
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.
Can we hide it from the screenshot via mask or css? We shouldn't be asserting browser styles for unstyled buttons in our tests
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.
Sure - hidden in screenshots now.
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.
Ditto
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.
Ditto
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.
Ditto
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 wouldn't expect an unstyled Close button here
t3chguy
left a comment
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.
It looks like my latest comment was not acknowledged or responded to.
|
Sorry, not sure what happened there: maybe github not updating, also possibly Friday afternoon brain syndrome. |
I'll leave the comment to do the explaining on the change itself.
This also adds axe testing to every compound web component... except the ones that have been excluded because they have errors. As per other comment, a lot of these are just that the stories don't put the components in their proper context. The tag excludes them until we have time to fix them all.
I've added a test for ReleaseAnnoucement when put on a div to assert that this axe failure goes away.