Skip to content

fix(Search Input): use popper to control the popup in advanced search #7621

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.

Sign up for GitHub

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

Merged
nicolethoen merged 3 commits into from
Jul 8, 2022
+262 −9
Merged

fix(Search Input): use popper to control the popup in advanced search #7621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb46af6
fix(SearchInput): use popper to control the popup in advanced search
bartoval Jun 27, 2022
da06c6a
refactor(Search input): update PR comments
bartoval Jul 6, 2022
f726d50
refactor(SearchInput): add unit tets
bartoval Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions packages/react-core/src/components/SearchInput/SearchInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as React from 'react';
import { css } from '@patternfly/react-styles';
import styles from '@patternfly/react-styles/css/components/SearchInput/search-input';
import { Button, ButtonVariant } from '../Button';
import { Badge } from '../Badge';
import AngleDownIcon from '@patternfly/react-icons/dist/esm/icons/angle-down-icon';
Expand All @@ -12,6 +11,7 @@ import ArrowRightIcon from '@patternfly/react-icons/dist/esm/icons/arrow-right-i
import { AdvancedSearchMenu } from './AdvancedSearchMenu';
import { TextInputGroup, TextInputGroupMain, TextInputGroupUtilities } from '../TextInputGroup';
import { InputGroup } from '../InputGroup';
import { Popper } from '../../helpers';

export interface SearchAttribute {
/** The search attribute's value to be provided in the search input's query string.
Expand Down Expand Up @@ -83,6 +83,13 @@ export interface SearchInputProps extends Omit<React.HTMLProps<HTMLDivElement>,
/** Delimiter in the query string for pairing attributes with search values.
* Required whenever attributes are passed as props */
advancedSearchDelimiter?: string;
/** The container to append the menu to.
* If your menu is being cut off you can append it to an element higher up the DOM tree.
* Some examples:
* appendTo={() => document.body}
* appendTo={document.getElementById('target')}
*/
appendTo?: HTMLElement | (() => HTMLElement) | 'inline';
}

const SearchInputBase: React.FunctionComponent<SearchInputProps> = ({
Expand Down Expand Up @@ -112,6 +119,7 @@ const SearchInputBase: React.FunctionComponent<SearchInputProps> = ({
nextNavigationButtonAriaLabel = 'Next',
submitSearchButtonLabel = 'Search',
isDisabled = false,
appendTo,
...props
}: SearchInputProps) => {
const [isSearchMenuOpen, setIsSearchMenuOpen] = React.useState(false);
Expand Down Expand Up @@ -277,11 +285,9 @@ const SearchInputBase: React.FunctionComponent<SearchInputProps> = ({

if (!!onSearch || attributes.length > 0 || !!onToggleAdvancedSearch) {
if (attributes.length > 0) {
return (
<div className={css(className, styles.searchInput)} ref={searchInputRef} {...props}>
{buildSearchTextInputGroupWithExtraButtons()}
const AdvancedSearch = (
<span>
Copy link
Copy Markdown
Member Author

@bartoval bartoval Jun 29, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@mcoker mcoker Aug 5, 2022

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Member Author

@bartoval bartoval Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor

@mcoker mcoker Aug 5, 2022

Choose a reason for hiding this comment

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

@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 😁

<AdvancedSearchMenu
className={styles.searchInputMenu}
Comment thread
wise-king-sullyman marked this conversation as resolved.
value={value}
parentRef={searchInputRef}
parentInputRef={searchInputInputRef}
Expand All @@ -298,8 +304,29 @@ const SearchInputBase: React.FunctionComponent<SearchInputProps> = ({
getAttrValueMap={getAttrValueMap}
isSearchMenuOpen={isSearchMenuOpen}
/>
</span>
);

const AdvancedSearchWithPopper = (
<div className={css(className)} ref={searchInputRef} {...props}>
<Popper
trigger={buildSearchTextInputGroupWithExtraButtons()}
popper={AdvancedSearch}
isVisible={isSearchMenuOpen}
enableFlip={true}
appendTo={() => appendTo || searchInputRef.current}
/>
</div>
);

const AdvancedSearchInline = (
<div className={css(className)} ref={searchInputRef} {...props}>
{buildSearchTextInputGroupWithExtraButtons()}
{AdvancedSearch}
</div>
);

return appendTo !== 'inline' ? AdvancedSearchWithPopper : AdvancedSearchInline;
}

return buildSearchTextInputGroupWithExtraButtons({ ...searchInputProps });
Expand Down
37 changes: 37 additions & 0 deletions packages/react-core/src/components/SearchInput/__tests__/SearchInput.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ describe('SearchInput', () => {
test('advanced search with custom attributes', () => {
const { asFragment } = render(
<SearchInput
data-testid="test-id"
attributes={[
{ attr: 'username', display: 'Username' },
{ attr: 'firstname', display: 'First name' }
Expand All @@ -102,7 +103,43 @@ describe('SearchInput', () => {

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

userEvent.click(screen.getByRole('button', { name: 'Open advanced search' }));
expect(screen.getByTestId('test-id')).toContainElement(screen.getByText('First name'))

expect(props.onSearch).toHaveBeenCalled();
expect(asFragment()).toMatchSnapshot();
});

test('advanced search with custom attributes and appendTo="inline', () => {
const {container} = render(
<SearchInput
data-testid="test-id"
attributes={[
{ attr: 'test', display: 'test' },
]}
appendTo='inline'
/>
);

userEvent.click(screen.getByRole('button', { name: 'Open advanced search' }));

expect(screen.getByTestId('test-id')).toContainElement(screen.getByText('test'))
});

test('advanced search with custom attributes and appendTo external DOM element', () => {
const {container} = render(
<SearchInput
data-testid="test-id"
attributes={[
{ attr: 'test', display: 'test' },
]}
appendTo={document.body}
/>
);

userEvent.click(screen.getByRole('button', { name: 'Open advanced search' }));

expect(screen.getByTestId('test-id')).not.toContainElement(screen.getByText('test'))
expect(document.body).toContainElement(screen.getByText('test'))
});
});
197 changes: 193 additions & 4 deletions ...s/react-core/src/components/SearchInput/__tests__/__snapshots__/SearchInput.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`SearchInput advanced search 1`] = `
<DocumentFragment>
<div
class="pf-c-search-input"
class=""
>
<div
class="pf-c-input-group"
Expand Down Expand Up @@ -125,7 +125,8 @@ exports[`SearchInput advanced search 1`] = `
exports[`SearchInput advanced search with custom attributes 1`] = `
<DocumentFragment>
<div
class="pf-c-search-input"
class=""
data-testid="test-id"
>
<div
class="pf-c-input-group"
Expand Down Expand Up @@ -194,9 +195,9 @@ exports[`SearchInput advanced search with custom attributes 1`] = `
</div>
<button
aria-disabled="false"
aria-expanded="false"
aria-expanded="true"
aria-label="Open advanced search"
class="pf-c-button pf-m-control"
class="pf-c-button pf-m-control pf-m-expanded"
data-ouia-component-id="OUIA-Generated-Button-control-8"
data-ouia-component-type="PF4/Button"
data-ouia-safe="true"
Expand Down Expand Up @@ -240,6 +241,194 @@ exports[`SearchInput advanced search with custom attributes 1`] = `
</svg>
</button>
</div>
<span
class=""
style="position: absolute; left: 0px; top: 0px; z-index: 9999; width: 0px;"
>
<div
class="pf-c-panel pf-m-raised"
>
<div
class="pf-c-panel__main"
>
<div
class="pf-c-panel__main-body"
>
<form
class="pf-c-form"
novalidate=""
>
<div
class="pf-c-form__group"
>
<div
class="pf-c-form__group-label"
>
<label
class="pf-c-form__label"
for="username_0"
>
<span
class="pf-c-form__label-text"
>
Username
</span>
</label>

</div>
<div
class="pf-c-form__group-control"
>
<input
aria-invalid="false"
class="pf-c-form-control"
data-ouia-component-id="OUIA-Generated-TextInputBase-1"
data-ouia-component-type="PF4/TextInput"
data-ouia-safe="true"
id="username_0"
type="text"
value="player"
/>
</div>
</div>
<div
class="pf-c-form__group"
>
<div
class="pf-c-form__group-label"
>
<label
class="pf-c-form__label"
for="firstname_1"
>
<span
class="pf-c-form__label-text"
>
First name
</span>
</label>

</div>
<div
class="pf-c-form__group-control"
>
<input
aria-invalid="false"
class="pf-c-form-control"
data-ouia-component-id="OUIA-Generated-TextInputBase-2"
data-ouia-component-type="PF4/TextInput"
data-ouia-safe="true"
id="firstname_1"
type="text"
value="john"
/>
</div>
</div>
<div
class="pf-c-form__group"
>
<div
class="pf-c-form__group-label"
>
<label
class="pf-c-form__label"
for="pf-random-id-2"
>
<span
class="pf-c-form__label-text"
>
Has words
</span>
</label>

</div>
<div
class="pf-c-form__group-control"
>
<input
aria-invalid="false"
class="pf-c-form-control"
data-ouia-component-id="OUIA-Generated-TextInputBase-3"
data-ouia-component-type="PF4/TextInput"
data-ouia-safe="true"
id="pf-random-id-2"
type="text"
value=""
/>
</div>
</div>
<div
class="pf-c-form__group"
>
<div
class="pf-c-form__group-control"
>
<button
aria-disabled="false"
class="pf-c-button pf-m-link pf-m-inline"
data-ouia-component-id="OUIA-Generated-Button-link-1"
data-ouia-component-type="PF4/Button"
data-ouia-safe="true"
type="button"
>
Link
<span
class="pf-c-button__icon pf-m-end"
>
<svg
aria-hidden="true"
fill="currentColor"
height="1em"
role="img"
style="vertical-align: -0.125em;"
viewBox="0 0 448 512"
width="1em"
>
<path
d="M448 80v352c0 26.51-21.49 48-48 48H48c-26.51 0-48-21.49-48-48V80c0-26.51 21.49-48 48-48h352c26.51 0 48 21.49 48 48zm-88 16H248.029c-21.313 0-32.08 25.861-16.971 40.971l31.984 31.987L67.515 364.485c-4.686 4.686-4.686 12.284 0 16.971l31.029 31.029c4.687 4.686 12.285 4.686 16.971 0l195.526-195.526 31.988 31.991C358.058 263.977 384 253.425 384 231.979V120c0-13.255-10.745-24-24-24z"
/>
</svg>
</span>
</button>
</div>
</div>
<div
class="pf-c-form__group pf-m-action"
>
<div
class="pf-c-form__group-control"
>
<div
class="pf-c-form__actions"
>
<button
aria-disabled="false"
class="pf-c-button pf-m-primary"
data-ouia-component-id="OUIA-Generated-Button-primary-1"
data-ouia-component-type="PF4/Button"
data-ouia-safe="true"
type="submit"
>
Search
</button>
<button
aria-disabled="false"
class="pf-c-button pf-m-link"
data-ouia-component-id="OUIA-Generated-Button-link-2"
data-ouia-component-type="PF4/Button"
data-ouia-safe="true"
type="reset"
>
Reset
</button>
</div>
</div>
</div>
</form>
</div>
</div>
</div>
</span>
</div>
</DocumentFragment>
`;
Expand Down