feat(SearchInput): enable animations #11874
Conversation
|
Preview: https://patternfly-react-pr-11874.surge.sh A11y report: https://patternfly-react-pr-11874-a11y.surge.sh |
kmcfaul
force-pushed
the
search-input-animations
branch
from
f50ebe4 to
dc0c86d
Compare
|
Looks fabulous! Only one comment - if I use the keyboard to close it, I don't see the focus ring on the search icon button once it's closed. In the existing version, that button got focus and you could just hit enter to re-expand the search input. |
There was a problem hiding this comment.
Other than the above regarding focus indicator + below nit, we should also add tests to ensure the new classes, attribute(s) (inert really), and markup is correct both by default and when opting into animations. I'd be fine with that as a followup if we prioritize it as part of code freeze just so we don't lose track of it
| onToggleExpand: (event: React.SyntheticEvent<HTMLButtonElement>, isExpanded: boolean) => void; | ||
| /** An accessible label for the expandable search input toggle. */ | ||
| toggleAriaLabel: string; | ||
| /** Flag indicating animations should be enabled when the search input expands and collapses. */ |
There was a problem hiding this comment.
Not a blocker, but may be worth noting this will alter the markup or something, and if we intend to remove this and have it be default behavior in the next breaking change then noting this will be removed in the next breaking change as well
|
Added a handful of tests for the new classes/inert. Re: the keyboard focus issue, this seems to be caused by the transition animation that plays on close. I've added a tiny timeout to the focus event and it seems to resolve it, but if you slow the animation down it happens still. I'm not sure if there's a better way to calc a timeout or wait for the animation. cc @thatblindgeye |
kmcfaul
force-pushed
the
search-input-animations
branch
from
ad399dd to
a31c987
Compare
| icon={<SearchIcon />} | ||
| onClick={onExpandHandler} | ||
| ref={searchInputExpandableToggleRef} | ||
| {...(isExpanded && { inert: '' })} |
There was a problem hiding this comment.
Just a comment, but if it's preferred, you could put inert on the <InputGroupItem /> and it will render everything in it as inert. There is only one one thing in it now, so there's no benefit to changing it, but I would probably put inert on the <InputGroupItem /> to indicate that the whole element should be ignored. Similar to if we conditionally rendered the expand toggle, I'd conditionally render the <InputGroupItem />, not just the button in it.
| <InputGroupItem isFill>{buildTextInputGroup()} </InputGroupItem> | ||
| <InputGroupItem isPlain>{expandableToggle}</InputGroupItem> | ||
| <InputGroupItem | ||
| isFill |
There was a problem hiding this comment.
Can you remove isFill from anywhere inputGroupStyles.modifiers.searchInput is defined? It's used to set flex-grow, and since we may need to change the flex-grow behavior down the road, I've moved it to the CSS instead of being tied to the markup to avoid a breaking change later.
| <InputGroup ref={triggerRef} {...searchInputProps}> | ||
| <InputGroupItem isFill>{buildTextInputGroup()}</InputGroupItem> | ||
| <InputGroupItem | ||
| isFill |
|
I'm still seeing |
|
Odd, I definitely updated that locally along with the inert changes but it didn't go in? Will reupdate. Edit: apparently I forgot to hit save before that commit 😆 |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
What: Closes #11855
Adds
hasAnimationstoexpandableInput(the props object that enables expand/collapse) and tied the new animations + structure to the flag. Also tweaks the extra buttons that can occur within a search input + advanced search input to account for expandable, since it is possible to currently pass theexpandableInputto other variants ofSearchInput.