Skip to content

Conversation

@AndrewMusgrave
Copy link
Member

WHY are these changes introduced?

This was noticed durning a bug hunt

WHAT is this pull request doing?

Removing the complexity around changing the clear buttons state so the clear button won't be visible to screen readers when the field is empty

Screenies

Before After
Screen Shot 2021-08-09 at 12 06 07 PM Screen Shot 2021-08-09 at 11 38 29 AM

🎩 checklist

Comment on lines -235 to -237
&.ClearButton-hidden {
@include visually-hidden;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason we need to hide the button rather than remove it? We could always add aria-hidden if so.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2021

size-limit report

Path Size
cjs 142.51 KB (-0.02% 🔽)
esm 96.27 KB (-0.04% 🔽)
esnext 139.44 KB (-0.06% 🔽)
css 33.74 KB (-0.03% 🔽)

@AndrewMusgrave AndrewMusgrave force-pushed the fix-clear-button-without-text branch from 5ab336f to 3f7dc8d Compare August 9, 2021 16:19
@AndrewMusgrave AndrewMusgrave marked this pull request as ready for review August 9, 2021 16:20
@AndrewMusgrave AndrewMusgrave requested a review from dleroux August 9, 2021 16:20
Co-authored-by: Daniel Leroux <dleroux@users.noreply.github.com>
@AndrewMusgrave AndrewMusgrave merged commit 949360c into main Aug 12, 2021
@AndrewMusgrave AndrewMusgrave deleted the fix-clear-button-without-text branch August 12, 2021 14:53
lucabezerra pushed a commit that referenced this pull request Aug 16, 2021
* Fully removed clear button when there is no text

* Fixed tests & added changelog

* Update UNRELEASED.md

Co-authored-by: Andrew Musgrave <AndrewMusgrave@users.noreply.github.com>

Co-authored-by: Daniel Leroux <dleroux@users.noreply.github.com>
BPScott pushed a commit that referenced this pull request Aug 17, 2021
* Fully removed clear button when there is no text

* Fixed tests & added changelog

* Update UNRELEASED.md

Co-authored-by: Andrew Musgrave <AndrewMusgrave@users.noreply.github.com>

Co-authored-by: Daniel Leroux <dleroux@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants