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

Merged
merged 1 commit into from
Nov 26, 2024

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
@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
@artonge artonge force-pushed the artonge/fix/name_prop_NcListItem branch 2 times, most recently from 6b36f07 to 245116d Compare November 21, 2024 10:22
src/components/NcListItem/NcListItem.vue Outdated Show resolved Hide resolved
@artonge artonge force-pushed the artonge/fix/name_prop_NcListItem branch from 245116d to dc1e4af Compare November 25, 2024 10:56
@artonge artonge changed the base branch from next to master November 25, 2024 10:57
@artonge artonge force-pushed the artonge/fix/name_prop_NcListItem branch 2 times, most recently from 7835614 to ba71cd2 Compare November 25, 2024 11:00
@artonge
Copy link
Contributor Author

artonge commented Nov 25, 2024

Targeting master again as there is no breaking change anymore

@artonge artonge force-pushed the artonge/fix/name_prop_NcListItem branch from ba71cd2 to a3a2ae5 Compare November 25, 2024 12:13
@artonge artonge requested review from nfebe and Pytal November 25, 2024 13:08
@artonge artonge merged commit 7400804 into master Nov 26, 2024
19 checks passed
@artonge artonge deleted the artonge/fix/name_prop_NcListItem branch November 26, 2024 15:43
@susnux
Copy link
Contributor

susnux commented Nov 26, 2024

/backport to next

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.

4 participants