Skip to content

Replace custom input field with NcTextField #40043

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.

Sign up for GitHub

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

Jump to bottom
Merged
JuliaKirschenheuter merged 1 commit into from
Aug 31, 2023
Merged

Replace custom input field with NcTextField #40043

JuliaKirschenheuter merged 1 commit into from
Aug 31, 2023

Conversation

@JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Aug 25, 2023 •
edited
Loading

Summary

Replace custom input field with NcInputField

Before After
Screenshot from 2023-08-25 15-55-45 Screenshot from 2023-08-28 14-50-33

Checklist

@JuliaKirschenheuter JuliaKirschenheuter added the 3. to review Waiting for reviews label Aug 25, 2023
@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Aug 25, 2023
ShGKme
ShGKme reviewed Aug 25, 2023
View reviewed changes
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.

Not really related to this PR, but should we also add aria-label and aria-haspopup to the emoji picker button here for a11y?

@JuliaKirschenheuter
Copy link
Contributor Author

JuliaKirschenheuter commented Aug 28, 2023

Not really related to this PR, but should we also add aria-label and aria-haspopup to the emoji picker button here for a11y?

@ShGKme could you please open new NcVue issue regarding this? Thanks a lot!

@ShGKme
Copy link
Contributor

ShGKme commented Aug 28, 2023 •
edited
Loading

@ShGKme could you please open new NcVue issue regarding this? Thanks a lot!

A trigger button is not a part of <NcEmojiPicker>, it's passed via slot. So those attrs should be set where the component is used.

Not sure about aria-label here because the button also has the text (emoji itself). Maybe it should be aria-description.

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37083-input_field_is_not_available_to_visually_impaired branch from 12ecdb1 to a953b16 Compare August 28, 2023 13:06
Pytal
Pytal reviewed Aug 28, 2023
View reviewed changes
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37083-input_field_is_not_available_to_visually_impaired branch from fc79503 to abbe949 Compare August 29, 2023 09:36
@JuliaKirschenheuter JuliaKirschenheuter mentioned this pull request Aug 29, 2023
Closed
@JuliaKirschenheuter JuliaKirschenheuter changed the title Replace custom input field with NcInputField Replace custom input field with NcTextField Aug 29, 2023
Pytal
Pytal reviewed Aug 29, 2023
View reviewed changes
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/37083-input_field_is_not_available_to_visually_impaired branch from abbe949 to 381e3ed Compare August 31, 2023 09:38
@JuliaKirschenheuter JuliaKirschenheuter merged commit aba3f99 into master Aug 31, 2023
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/37083-input_field_is_not_available_to_visually_impaired branch August 31, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pytal Pytal Pytal left review comments

@artonge artonge artonge approved these changes

@ShGKme ShGKme ShGKme approved these changes

@susnux susnux Awaiting requested review from susnux

Assignees

@JuliaKirschenheuter JuliaKirschenheuter

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

No milestone

5 participants

@JuliaKirschenheuter @ShGKme @artonge @Pytal