Skip to content
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

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Oct 2, 2024

As it is only required if the slot is missing.

@artonge artonge self-assigned this Oct 2, 2024
@artonge artonge added bug Something isn't working 3. to review Waiting for reviews labels Oct 2, 2024
Copy link
Contributor

@ShGKme ShGKme left a 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

@artonge
Copy link
Contributor Author

artonge commented Oct 2, 2024

One problem, it is used in computedActionsAriaLabel

So let's announce it as primarily used for accessibility, and also as fallback for the name slot? And keep it mandatory?

@ShGKme
Copy link
Contributor

ShGKme commented Oct 2, 2024

One problem, it is used in computedActionsAriaLabel

So let's announce it as primarily used for accessibility, and also as fallback for the name slot? And keep it mandatory?

I'd say, we should just simplify computedActionsAriaLabel. We don't need to duplicate location there, especially when it's long. It's a menu in a specific list item and that enough.

@artonge
Copy link
Contributor Author

artonge commented Oct 3, 2024

I'd say, we should just simplify computedActionsAriaLabel. We don't need to duplicate location there, especially when it's long. It's a menu in a specific list item and that enough.

So dropping computedActionsAriaLabel, using actionsAriaLabel directly, and making it required? Not sure what is the state regarding enforcing accessibility properties.

Edit: pushed the mentioned changes.

@artonge artonge force-pushed the artonge/fix/name_prop_NcListItem branch from 0734559 to deef42b Compare October 3, 2024 12:40
@ShGKme ShGKme requested a review from susnux October 3, 2024 18:06
@ShGKme ShGKme changed the title fix: Don't require name prop in NcListItem fix(NcListItem): Don't require name prop Oct 3, 2024
@artonge artonge requested a review from skjnldsv October 8, 2024 16:14
@@ -641,7 +640,7 @@ export default {
*/
actionsAriaLabel: {
type: String,
default: '',
required: true,
Copy link
Contributor

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:

Suggested change
required: true,
default: t('Actions'),

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

@artonge artonge Oct 9, 2024

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artonge As @susnux mentioned, we don't need to require aria label here. NcActions has the default label already, and it works fine by default. So there is no need to make users always create a name for the menu when using NcListItem and make it a breaking change.

Copy link
Contributor Author

@artonge artonge Oct 10, 2024

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

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

@artonge artonge changed the base branch from master to next October 9, 2024 15:17
@artonge artonge force-pushed the artonge/fix/name_prop_NcListItem branch from deef42b to 993193e Compare October 9, 2024 15:21
@artonge artonge requested a review from susnux October 9, 2024 15:22
@ShGKme ShGKme self-requested a review October 10, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants