feat(SearchInput): new expandable button for masthead variant by andyyvo · Pull Request #7903 · patternfly/patternfly-react

andyyvo · 2022-08-30T07:51:25Z

What: Closes #7380 . Introduced two new props isExpanded and onToggleExpand in SearchInput.tsx. These new props allow the search input component to expand and collapse via a button. This wrapper can be found in the buildSearchTextInputGroupWithExpand and buildSearchTextInputGroupWithExtraButtonsAndExpand functions where the former adds the button to the basic search input and the latter adds the button to the search input with submit button. An example was created that demonstrates the use of a basic search input with the expand toggle button.

Additional issues: Code regarding the advanced search is repeated. Writing it as a function messed up something about the state so it was simply copied twice.

patternfly-build · 2022-08-30T08:02:02Z

Preview: https://patternfly-react-pr-7903.surge.sh

A11y report: https://patternfly-react-pr-7903-a11y.surge.sh

nicolethoen

@mcarrano Should we add a new masthead demo to include this search input feature as past of this issue? Or is there a follow up issue?

Also, @mmenestr or @mcarrano, I feel like it looks a little strange once I start typing in the search input because there are two close buttons next to each other - one for clearing the input and one for collapsing the search. Is that something we considered?

nicolethoen · 2022-08-31T20:54:08Z


+  const expandedLabel = () => {
+    if (isExpanded) {
+      return 'Close';


These need to be something the consumer can override - consider internationalization where people need to be able to translate their sites into different languages.

Going off of Nicole's comment above, the default value for the "close" button could be more similar to button in the collapsed state, e.g. "Close search".

Though because this is something that can collapse/expand, I'm thinking an alternative would be to omit an aria-label that includes the state (though not getting rid of it entirely), and just use aria-expanded on the button to announce the expanded state.

Going that route would still allow a consumer to pass in an aria-label to this button via a new prop, then aria-expanded can be set based on the isExpanded prop. @nicolethoen wdyt?

+1 about aria-expanded with a separate ariaLabel type prop.

Small nit is that I would prefer this to not be a function (an inline if block should work the same), but either way.

nicolethoen · 2022-08-31T20:55:46Z

+    if (!!onSearch || attributes.length > 0 || !!onToggleAdvancedSearch) {
+      if (attributes.length > 0) {
+        const AdvancedSearch = (
+          <span>


Is this code simply repeated from the code around lines 333? If so, we should avoid duplicate code as much as possible.

mcarrano · 2022-09-01T16:21:38Z

@andyyvo @nicolethoen Yes, I think it would be useful to add a demo and no, I didn't already open an issue for that.

Regarding the two close/clear buttons, yes, I do think that was an oversight and we shouldn't have that. We basically want this to work like the search in the masthead on the PatternFly site. There, the field does not have a separate clear, but collapsing the field also clears the field. Could this be made to work that way?

Finally comment/question. I found it a bit confusing that the example starts with the field expanded because it's not obvious how to expand it. Maybe start with the search field hidden?

nicolethoen · 2022-09-01T16:27:21Z

@mcarrano Should collapsing the search input also clear the input? so when it expands again, it's empty?

mcarrano · 2022-09-01T19:46:52Z

@mcarrano Should collapsing the search input also clear the input? so when it expands again, it's empty?

Yes, I think so.

thatblindgeye · 2022-09-01T16:31:30Z

  const [searchValue, setSearchValue] = React.useState(value);
  const searchInputRef = React.useRef(null);
  const searchInputInputRef = innerRef || React.useRef(null);
+  const [isSearchInputExpanded, setIsSearchInputExpanded] = React.useState(true);


Could we omit this state entirely and base logic elsewhere off of the isExpanded prop? It doesn't seem like it's entirely needed since consumers will have to pass in both the isExpanded and onToggleExpand props to control the expanding functionality.

I think it could be removed. The state could be used to enable uncontrolled expansion, but as is it doesn't appear to work after playing with the example (onToggleExpand is required to turn the input into an expandable variant, and removing isExpanded prevents expansion entirely).

if we are going the support uncontrolled, I would update the comment to mention that.

thatblindgeye · 2022-09-01T16:34:37Z


+  const expandedLabel = () => {
+    if (isExpanded) {
+      return 'Close';


Going off of Nicole's comment above, the default value for the "close" button could be more similar to button in the collapsed state, e.g. "Close search".

Though because this is something that can collapse/expand, I'm thinking an alternative would be to omit an aria-label that includes the state (though not getting rid of it entirely), and just use aria-expanded on the button to announce the expanded state.

Going that route would still allow a consumer to pass in an aria-label to this button via a new prop, then aria-expanded can be set based on the isExpanded prop. @nicolethoen wdyt?

thatblindgeye · 2022-09-01T16:53:05Z

+
+export const SearchInputWithExpandable: React.FunctionComponent = () => {
+  const [value, setValue] = React.useState('');
+  const [isExpanded, setIsExpanded] = React.useState(true);


Similar to what @mcarrano said, I'd opt to have this false by default

mcoker

LGTM! If you're going to add a demo in the masthead, part of the core PR adds a .pf-m-plain variation to the input group that removes the white background for use against a non-white background, otherwise it will look like this:

tlabaj · 2022-09-12T20:33:32Z

@mcarrano @nicole I opened issue #7968 to add the new demo

wise-king-sullyman · 2022-09-13T14:22:19Z

I'm buttoning this work up now, as far as the API goes do you all have any thoughts on having separate props for isExpanded, onToggleExpanded, and a new aria label prop for the toggle vs combining all those props into a single object prop? @tlabaj @thatblindgeye @nicolethoen @kmcfaul

nicolethoen · 2022-09-13T14:33:02Z

I like them combined into a single, well documented prop - especially if we expect people to need to use more than one at a time to implement the feature.

thatblindgeye · 2022-09-13T14:59:41Z

Agreed with Nicole's comment. It maaaay also differentiate those props a little more from some other props (aria-label, onToggleAdvancedSearch)

tlabaj · 2022-09-13T15:52:59Z

I like the idea of grouping props together that otherwise do not make sense on there own like in this case.

thatblindgeye

Had a few comments below, two related to the a11y error that'll need an update, and one that's non-blocking/I can create a followup for

thatblindgeye · 2022-09-13T16:27:53Z

+      value={value}
+      onChange={onChange}
+      onClear={() => onChange('')}
+      expandableProps={{ isExpanded, onToggleExpand }}


This needs the toggleAriaLabel prop passed in.

thatblindgeye · 2022-09-13T16:42:47Z

+  const onExpandHandler = (event: React.SyntheticEvent<HTMLButtonElement>) => {
+    setSearchValue('');
+    onToggleExpand(isExpanded, event);
+  };


Not blocking and more of a followup issue since it'll depend whether the behavior should be built-in or just part of the example, but we should tidy up the focus management when the search input in the new example expands and collapses. Currently the behavior makes it difficult to know what has focus after interaction:

Expandable.search.input.focus.mov

thatblindgeye · 2022-09-13T16:50:05Z

+  /** Callback function to toggle the expandable search input */
+  onToggleExpand: (isExpanded: boolean, event: React.SyntheticEvent<HTMLButtonElement>) => void;
+  /** An accessible label for the expandable search input toggle */
+  toggleAriaLabel?: string;


Because an a11y error will get flagged for the icon button not having an accessible name, we should make this a required prop as well.

thatblindgeye

🔥 Awesome job on this to both of you, @andyyvo and @wise-king-sullyman !

wise-king-sullyman · 2022-09-13T19:23:47Z

@mcarrano does the behavior in the example now line up with what you expected?

Convenience link: https://patternfly-react-pr-7903.surge.sh/components/search-input#with-expandable-button

mcarrano

@mcarrano does the behavior in the example now line up with what you expected?

Yes. This looks great. Thanks @andyyvo @wise-king-sullyman

tlabaj · 2022-09-14T13:25:18Z

  hint?: string;
-
+  /** Object that makes the search input expandable/collapsible */
+  expandableProps?: expandableProps;


As discussed on slack. I change this prop name to not be suffixed with "Props".

tlabaj

LGTM. Thank you Andy and Austin.

patternfly-build · 2022-09-14T15:03:44Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.77.0
@patternfly/react-catalog-view-extension@4.89.0
@patternfly/react-charts@6.91.0
@patternfly/react-code-editor@4.79.0
@patternfly/react-console@4.89.0
@patternfly/react-core@4.238.0
@patternfly/react-docs@5.99.0
@patternfly/react-icons@4.89.0
@patternfly/react-inline-edit-extension@4.83.0
demo-app-ts@4.198.0
@patternfly/react-integration@4.200.0
@patternfly/react-log-viewer@4.83.0
@patternfly/react-styles@4.88.0
@patternfly/react-table@4.107.0
@patternfly/react-tokens@4.90.0
@patternfly/react-topology@4.85.0
@patternfly/react-virtualized-extension@4.85.0
transformer-cjs-imports@4.76.0

Thanks for your contribution! 🎉

feat(SearchInput): new expandable button for masthead variant

965bcb9

nicolethoen requested changes Aug 31, 2022

View reviewed changes

tlabaj requested review from mcarrano, mcoker and mmenestr September 1, 2022 15:30

tlabaj added ux review labels Sep 1, 2022

thatblindgeye requested changes Sep 1, 2022

View reviewed changes

mcoker reviewed Sep 8, 2022

View reviewed changes

tlabaj mentioned this pull request Sep 12, 2022

[Masthead] - [Add search input demo] #7968

Closed

wise-king-sullyman added 8 commits September 12, 2022 17:36

adjust prop descriptions and default values

663c046

refactor to remove repeated code

df32432

remove uncontrolled usage support

95e8996

clear search input value when expansion is toggled

3c54185

revise toggle a11y attributes

1b4ead6

initialize example with the input unexpanded

dc86eac

conditionally render TextInputGroupUtilities to prevent whitespace

8c719d6

Merge branch 'main' into iss7380

8257ec9

refactor API to use combined prop

bd43d56

add tests

3ed0886

mcoker approved these changes Sep 13, 2022

View reviewed changes

thatblindgeye requested changes Sep 13, 2022

View reviewed changes

fix a11y issues

1619ac3

wise-king-sullyman requested review from nicolethoen and thatblindgeye September 13, 2022 18:15

thatblindgeye approved these changes Sep 13, 2022

View reviewed changes

wise-king-sullyman requested a review from tlabaj September 13, 2022 19:23

mcarrano approved these changes Sep 13, 2022

View reviewed changes

nicolethoen approved these changes Sep 13, 2022

View reviewed changes

tlabaj reviewed Sep 14, 2022

View reviewed changes

edit prop name

6b0ab06

wise-king-sullyman requested a review from tlabaj September 14, 2022 14:17

tlabaj approved these changes Sep 14, 2022

View reviewed changes

tlabaj merged commit d2a6c47 into patternfly:main Sep 14, 2022

Conversation

andyyvo commented Aug 30, 2022

Uh oh!

patternfly-build commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarrano commented Sep 1, 2022

Uh oh!

nicolethoen commented Sep 1, 2022

Uh oh!

mcarrano commented Sep 1, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmcfaul Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Sep 12, 2022

Uh oh!

wise-king-sullyman commented Sep 13, 2022

Uh oh!

nicolethoen commented Sep 13, 2022

Uh oh!

thatblindgeye commented Sep 13, 2022

Uh oh!

tlabaj commented Sep 13, 2022

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman commented Sep 13, 2022

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Sep 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

patternfly-build commented Aug 30, 2022 •

edited

Loading

kmcfaul Sep 9, 2022 •

edited

Loading