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

Feat: move remove filter button and display it on hover #8217

Closed
wants to merge 3 commits into from

Conversation

thibault-barrat
Copy link
Contributor

@thibault-barrat thibault-barrat commented Sep 30, 2022

  • Display the close button only when hovering the input
  • Move the close button to the top right of the input
Enregistrement.de.l.ecran.2022-12-01.a.15.08.39.mov

@thibault-barrat thibault-barrat added the RFR Ready For Review label Sep 30, 2022
Copy link
Contributor

@WiXSL WiXSL left a comment

Choose a reason for hiding this comment

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

I really like it! 👍

@slax57
Copy link
Contributor

slax57 commented Oct 3, 2022

IMO this should be an opt-in feature, because it is a regression in terms of accessibility.

Besides, if it's a new feature, it should target the next branch.

@fzaninotto
Copy link
Member

I like it!

Why do you see it as a regression in accessibility?

In my opinion, this feature needs a bit of visual polish on the background and position of the close button, the hover zone, and the tooltip.

@slax57
Copy link
Contributor

slax57 commented Oct 4, 2022

I like it!

Why do you see it as a regression in accessibility?

In my opinion, this feature needs a bit of visual polish on the background and position of the close button, the hover zone, and the tooltip.

Because you can no longer use the Clear button with keyboard only

@antoinefricker
Copy link
Contributor

antoinefricker commented Dec 1, 2022

I make a reservation on 3 points:

  • filter UI do not display its clearable state without hovering it; I would prefer the button to be displayed by default
  • chips are now clickable, without consequent action; they have the UI of a toggler (enabled/disabled) but without being one
  • dark theme should also be evaluated

Base automatically changed from next to master March 24, 2023 10:58
@fzaninotto
Copy link
Member

After thinking about the accessibility regression, we won't move forward with this change. We'll keep the less visually appealing "always on" clear button to remain accessible.

thanks for your contribution anyway!

@fzaninotto fzaninotto closed this Aug 28, 2023
@fzaninotto
Copy link
Member

Superseded by #9224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants