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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

enh(NcSelect): Add visible input label #4963

Merged
merged 5 commits into from Jan 3, 2024
Merged

enh(NcSelect): Add visible input label #4963

merged 5 commits into from Jan 3, 2024

Conversation

Pytal
Copy link
Contributor

@Pytal Pytal commented Dec 15, 2023

Summary

馃弫 Checklist

  • 鉀戯笍 Tests are included or are not applicable
  • 馃摌 Component documentation has been extended, updated or is not applicable

@Pytal Pytal added enhancement New feature or request 3. to review Waiting for reviews feature: select Related to the NcSelect* components accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Dec 15, 2023
@Pytal Pytal added this to the 8.4.0 milestone Dec 15, 2023
@Pytal Pytal self-assigned this Dec 15, 2023
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Overall, fantastic PR. Just would recommend squashing the first, third, and fourth PR's together because they are to do with the actual template, and leave the second commit as it, because its to do with doc descriptions : )

heres a link on how to squash the commits even if they are not in order - dunno if you need this but I personally did not know you could squash out of order commits. Once that's done, this PR is good!

@Pytal
Copy link
Contributor Author

Pytal commented Dec 15, 2023

Overall, fantastic PR. Just would recommend squashing the first, third, and fourth PR's together because they are to do with the actual template, and leave the second commit as it, because its to do with doc descriptions : )

I'd like to keep the current logical separation of commits, will be a merge commit anyways :)

@Pytal Pytal requested a review from emoral435 December 15, 2023 20:50
@ShGKme
Copy link
Contributor

ShGKme commented Dec 15, 2023

If I understand all correctly, with a new prop, we have two props that have a meaning of the component label:

  1. inputLabel - visible label
  2. ariaLabelCombobox - invisible label

And to avoid a11y issues:

  • They should never have different value
  • At least one should be present (or the label is outside and inputId is present)

I think we should either add a validation here to check both conditions, or we should mark ariaLabelCombobox deprecated and do the same what we have with Nc*Field to mark if label should be visible, or not, or it is outside.

src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
src/components/NcSelect/NcSelect.vue Show resolved Hide resolved
@Pytal
Copy link
Contributor Author

Pytal commented Dec 16, 2023

Warnings added without the deprecation as I think we should still allow setting ariaLabelCombobox, but todos have been added for the next major @ShGKme

@Pytal Pytal requested a review from ShGKme December 16, 2023 00:49
@susnux
Copy link
Contributor

susnux commented Dec 16, 2023

Should we not use the same design as for the input boxes, text area ...?

Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

Maybe what @susnux said is best, but for now, this LGTM

src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
@ShGKme ShGKme modified the milestones: 8.4.0, 8.5.0 Dec 27, 2023
@Pytal Pytal force-pushed the enh/a11y/select-label branch 2 times, most recently from 171d8f4 to e755a0b Compare January 3, 2024 02:05
@Pytal
Copy link
Contributor Author

Pytal commented Jan 3, 2024

Should we not use the same design as for the input boxes, text area ...?

Can be done separately 馃拝

@Pytal Pytal requested a review from ShGKme January 3, 2024 02:11
src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
src/components/NcSelect/NcSelect.vue Show resolved Hide resolved
Signed-off-by: Christopher Ng <chrng8@gmail.com>
- Remove unnecessary input id

Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 3, 2024
@Pytal Pytal merged commit 8bf8598 into master Jan 3, 2024
16 checks passed
@Pytal Pytal deleted the enh/a11y/select-label branch January 3, 2024 23:15
@Pytal Pytal mentioned this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility Making sure we design for the widest range of people possible, including those who have disabilities enhancement New feature or request feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants