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

Allow 'search' as type prop for NcTextField #3190

Merged
merged 3 commits into from
Sep 20, 2022
Merged

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Sep 7, 2022

@artonge artonge added enhancement New feature or request 3. to review Waiting for reviews labels Sep 7, 2022
@artonge artonge added this to the 7.0.0 milestone Sep 7, 2022
@artonge artonge self-assigned this Sep 7, 2022
Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

This adds a second clear button at the trailing end. Not sure how we should go forward here.

Screenshot 2022-09-12 at 08 23 27

One drawback is that this native button cannot be focused.

Maybe if the devs pass in the "search" type we can capture it instead of forwarding it to the input component and take care of the functionality and looks ourselves. So basically displaying the trailing close button and the leading search icon.
Any other ideas?

Signed-off-by: Louis <6653109+artonge@users.noreply.github.com>
@artonge
Copy link
Contributor Author

artonge commented Sep 12, 2022

This adds a second clear button at the trailing end. Not sure how we should go forward here.

Nice catch. I have added a condition to the trainling icon slot. What do you think ?

@marcoambrosini
Copy link
Contributor

I think that our close button is better because

  • click target it is big (44px)
  • The cursor becomes a pointer when hovering it
  • it is focusable
  • proper style - uses a mdi icon
Screen.Recording.2022-09-12.at.09.29.17.mov

What would be the advantages of using the native one?

@artonge
Copy link
Contributor Author

artonge commented Sep 12, 2022

I think that our close button is better because

* click target it is big (44px)

* The cursor becomes a pointer when hovering it

* it is focusable

* proper style - uses a mdi icon

Screen.Recording.2022-09-12.at.09.29.17.mov

What would be the advantages of using the native one?

If the goal is to have a proper type for the input, and if the browser's close icon is not removable, then I don't see any other solution.

@jancborchardt any opinion on the accessibility benefit of using a proper type for search input ? Maybe it is not worth the trouble

Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@artonge artonge merged commit 3264533 into master Sep 20, 2022
@artonge artonge deleted the fix/type_prop_NcTextField branch September 20, 2022 14:00
@Pytal Pytal mentioned this pull request Sep 21, 2022
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants