-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix(NcListItem): Don't require name prop #6128
base: next
Are you sure you want to change the base?
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.
One problem, it is used in computedActionsAriaLabel
So let's announce it as primarily used for accessibility, and also as fallback for the |
I'd say, we should just simplify |
So dropping Edit: pushed the mentioned changes. |
0734559
to
deef42b
Compare
@@ -641,7 +640,7 @@ export default { | |||
*/ | |||
actionsAriaLabel: { | |||
type: String, | |||
default: '', | |||
required: true, |
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 would consider this more or less a breaking change 👀 As this is more strict.
Maybe just add a note that this will be required in v9 and instead of making it required add:
required: true, | |
default: t('Actions'), |
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.
True, I would be fine if we merge that in v9 only. I don't think a default aria label would be a good thing, this would mess up accessibility.
Is v9 the next
branch?
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.
leaving it unset will trigger the default NcActions
label which is also just Actions
so this would also be fine
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.
Is v9 the
next
branch?
yes
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.
Alright thanks, changed branch to target next.
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.
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.
But having a default "Actions" label for accessibility is not really accessible, no? I would rather force dev to set an aria label, than have x elements with the same description on the same page
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.
But having a default "Actions" label for accessibility is not really accessible, no?
Why not?
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.
have x elements with the same description on the same page
It is not a problem, they have different contexts. It is not an "Actions" button somewhere on the page. It is a list item, and withing the list item there is a menu button with actions called "Actions". In a general case it's enough.
When a person uses only a screen-reader with no screen, they go to this button throw the page and list items. If a person uses a screen together with assistive technologies, then they know with which part of the UI they interact.
Like if we have a normal button to delete a list item in the list item with a normal text content, we don't write text "Delete user X" or "Delete list item #125". We just call it a "Delete" button.
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.
Best rule I got told: Both should have the same information (visual vs accessible).
So we have visual information: Actions button.
So accessible information "Actions" would be ok. The context is already provided by the wrapping element.
Signed-off-by: Louis Chemineau <[email protected]>
deef42b
to
993193e
Compare
As it is only required if the slot is missing.