fix(Search Input): use popper to control the popup in advanced search by bartoval · Pull Request #7621 · patternfly/patternfly-react

bartoval · 2022-06-28T13:51:33Z

What: Closes #7592

patternfly-build · 2022-06-28T14:02:38Z

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

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

bartoval · 2022-06-29T20:04:05Z

-        <div className={css(className, styles.searchInput)} ref={searchInputRef} {...props}>
-          {buildSearchTextInputGroupWithExtraButtons()}
+      const advancedSearch = (
+        <span>


we need a native HTML markup here, otherwise, the popper is not going to calculate the position and set the item absolute

nicolethoen · 2022-07-05T17:16:22Z

+            trigger={buildSearchTextInputGroupWithExtraButtons({ id: 'custom-advanced-search' })}
+            popper={advancedSearch}
+            isVisible={isSearchMenuOpen}
+            enableFlip={false}


I think we should enableFlip=true by default

nicolethoen · 2022-07-05T17:18:54Z

+            popper={advancedSearch}
+            isVisible={isSearchMenuOpen}
+            enableFlip={false}
+            appendTo={() => document.querySelector('#custom-advanced-search')}


We cannot hardcode this value. It should be something that is configurable the prop could be the same or inspired by the appendTo prop from TimePicker.tsx

/** The container to append the advanced search form to. Defaults to 'parent'. * If your form is being cut off you can append it to an element higher up the DOM tree. * Some examples: * appendTo="parent" * appendTo={() => document.body} * appendTo={document.getElementById('target')} */ appendTo?: HTMLElement | (() => HTMLElement) | 'inline' | 'parent';

I am not sure about that.
We want to append this menu to 'SearchTextInputGroup' (we set the value 'custom-advanced-search' as id) every time and not to an external DOM element.

Maybe I am missing something.
Could you please provide an example of where we need to configure this element outside the component?

It's possible that someone will put an advanced search into a Wizard modal for example and that would get clipped or formatted incorrectly relative to the wizard or modal. Those tend to be some the edge cases we are allowing for.

Additionally, if someone wants an inline advanced search, where opening the modal does (for whatever reason) push down the contents of the page so the advanced search is not overlaying the results, that should be possible using the 'inline' value of 'appendTo'

Got it, thanks! 👍🏼
So I think we can:

keep the current behavior as default (without passing the prop appendTo). (popper)

provide the inline option to push down the content ( no popper)

add the possibility to pass an external DOM element (popper)

nicolethoen · 2022-07-06T12:47:56Z

+   * menuAppendTo={() => document.body}
+   * menuAppendTo={document.getElementById('target')}
+   */
+  menuAppendTo?: HTMLElement | (() => HTMLElement) | 'inline';


I think the prop name menuAppendTo is a little misleading since the Menu is a component we have and we are not using a Menu in this case, but a panel. So I think it could be panelAppendTo or appendTo or maybe something else.

nicolethoen

sorry, I have one request to update the prop name

wise-king-sullyman

This looks great! One request: can you add a couple of tests to verify the behavior of appendTo? After that I'll be ready to approve 🙂

wise-king-sullyman

Just a couple nits!

wise-king-sullyman · 2022-07-08T14:31:19Z

    userEvent.click(screen.getByRole('button', { name: 'Search' }));

+    userEvent.click(screen.getByRole('button', { name: 'Open advanced search' }));
+    expect(screen.getByTestId('test-id').querySelector('.pf-c-panel')).toBeInTheDocument()


Nit: rather than using querySelector + toBeInTheDocument to determine where the search menu is being placed in the DOM can you use an RTL query to select the form (or something in it) and maybe the toContainElement matcher?

Reason being that I believe this would better replicate the actual user experience with the component.

Good idea. Thanks 👍🏼

wise-king-sullyman

Looks great! 🚀

…patternfly#7621) * fix(SearchInput): use popper to control the popup in advanced search * refactor(Search input): update PR comments * refactor(SearchInput): add unit tets

mcoker · 2022-08-05T00:55:06Z

-        <div className={css(className, styles.searchInput)} ref={searchInputRef} {...props}>
-          {buildSearchTextInputGroupWithExtraButtons()}
+      const AdvancedSearch = (
+        <span>


@bartoval Apologies for the super late comment, I was following up on something I made a note about a while ago. FWIW with the original issue, we should always try and avoid applying multiple component styles to an element (ie, .pf-c-panel.pf-c-search-intput__menu).

This element should be a <div> though, a <span> is an inline element and its permitted content is phrasing content. So technically it's invalid HTML to put non-phrasing content in a <span>, though it works just fine. Created an issue for that here - #7796

Also it's worth noting that appendTo="inline" just drops the panel in the layout and pushes everything below it down (it doesn't display on top of the stuff below it). A few questions around that:

Should appendTo be menuAppendTo like the other components that have a menu in them that popper can position?

Is that the intended behavior of appendTo="inline" or should that display on top of the stuff below it like it used to - it just won't use popper. Not sure how we'd do that if we want to continue to use the panel component there, but that would be inline with the other menus that support menuAppendTo.

If we are going to leave it so that it pushes the stuff below the panel down, should we remove the "raised" modifier from the panel since the shadow implies depth/overlay?

@mcoker thanks for your comments! They are very interesting.

I will pick up the issue

Regarding your questions:

there is comment above about that and using menuAppendTo is a little misleading since the Menu is a component we have and we are not using a Menu in this case.

Yes this behaviour is predicted like the TimePicker (@nicolethoen )

Not sure about it.

@bartoval thanks!!

there is comment above about that and using menuAppendTo is a little misleading since the Menu is a component we have and we are not using a Menu in this case.

👍 got it. I agree, that could be confusing. I would probably argue that menu is more of a concept there to refer to the thing that pops open in the component, and that could be used with whatever component pops up. It seems like it might be more predictable/consistent that way. As in if we stopped using the "menu" component and used some other component for the thing that pops up, menuAppendTo would still be fine and wouldn't necessarily need to change IMO since we shouldn't be tying the prop name to the component used in the first place and we're using menu as a concept. Also I'd probably say that [menu/thing]AppendTo would be used on the component that has a thing that pops up, and appendTo would be more appropriate on the thing that pops up itself. Just my $0.02 😁

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch from 783c3c8 to 346efb7 Compare June 28, 2022 14:04

bartoval self-assigned this Jun 28, 2022

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch 2 times, most recently from 3cfdc2e to 4fd7d62 Compare June 29, 2022 19:49

fix(SearchInput): use popper to control the popup in advanced search

cb46af6

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch from 4fd7d62 to cb46af6 Compare June 29, 2022 20:02

bartoval commented Jun 29, 2022

View reviewed changes

bartoval requested review from nicolethoen and tlabaj June 29, 2022 20:33

nicolethoen requested changes Jul 5, 2022

View reviewed changes

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch 3 times, most recently from 64be51d to 83c784e Compare July 6, 2022 08:15

nicolethoen approved these changes Jul 6, 2022

View reviewed changes

nicolethoen requested changes Jul 6, 2022

View reviewed changes

refactor(Search input): update PR comments

da06c6a

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch from 83c784e to da06c6a Compare July 6, 2022 14:00

bartoval requested a review from nicolethoen July 6, 2022 14:01

wise-king-sullyman reviewed Jul 6, 2022

View reviewed changes

Comment thread packages/react-core/src/components/SearchInput/SearchInput.tsx

wise-king-sullyman requested changes Jul 7, 2022

View reviewed changes

bartoval requested a review from wise-king-sullyman July 8, 2022 07:33

wise-king-sullyman requested changes Jul 8, 2022

View reviewed changes

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch 2 times, most recently from eafa708 to 9cd5fb7 Compare July 8, 2022 15:19

bartoval requested a review from wise-king-sullyman July 8, 2022 15:20

refactor(SearchInput): add unit tets

f726d50

bartoval force-pushed the advanced_filter_search_position_no_longer_absolute branch from 9cd5fb7 to f726d50 Compare July 8, 2022 15:40

kmcfaul approved these changes Jul 8, 2022

View reviewed changes

wise-king-sullyman approved these changes Jul 8, 2022

View reviewed changes

nicolethoen approved these changes Jul 8, 2022

View reviewed changes

nicolethoen merged commit 13a8d5f into patternfly:main Jul 8, 2022

mcoker reviewed Aug 5, 2022

View reviewed changes

Conversation

bartoval commented Jun 28, 2022

Uh oh!

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

Uh oh!

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!

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!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman 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!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartoval Aug 5, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented Jun 28, 2022 •

edited

Loading

bartoval Aug 5, 2022 •

edited

Loading