Skip to content
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.

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

[Euisearchbar] Add option to render hint #6319

Merged
merged 10 commits into from
Oct 28, 2022

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Oct 20, 2022

In this PR I've added an option to the <EuiSearchBar /> component to render a hint below. By default it displays underneath the bar on focus but its visibility can also be controlled using the isOpen prop of the <EuiInputPopover /> component.

Screenshot 2022-10-20 at 09 22 16

Screenshot 2022-10-20 at 09 22 32

Fixes #6005

@sebelga sebelga self-assigned this Oct 20, 2022
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6319/

@sebelga
Copy link
Contributor Author

sebelga commented Oct 20, 2022

jenkin test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6319/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6319/

@chandlerprall chandlerprall self-requested a review October 20, 2022 17:34
@cee-chen
Copy link
Member

Assigning @1Copenut as well to take a look at this PR and QA that it meets accessibility standards/requirements

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I think this hint is a net gain for our search bar, much appreciated.

I think we should adjust the search bar labels in our docs examples to be shorter, something like "Query string search" or "Search bar". The current label reads more like instructions how to use the input, and should be moved into the hint.

Let's have the hint text be less similar to the placeholder so we illustrate the difference and value in having it as a third instruction set beyond label and placeholder.

src/components/search_bar/search_bar.tsx Show resolved Hide resolved
src/components/search_bar/search_bar.tsx Show resolved Hide resolved
/**
* Hint to render below the search bar
*/
hint?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

The hint needs to have its tabindex removed or set to "-1". Right now it has a tabindex of "0" that allows it to take keyboard focus, but because we're using a portal, it's not a sibling element to the input it represents. This situation could be confusing for users who navigate the screen with sight and assistive technology together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this something that should be handled globally by the <EuiInputPopover /> component for every consumer who want to display a popover underneath an <input /> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into it, the z-index is set to 2000. Setting it to -1 is not possible as it hides the popover. If this needs to be changed globally for all consumers of the EuiInputPopover I'd prefer it to be a different PR as I don't know the impact if could have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I was looking at tabindex in the HTML structure. I agree z-index needs to remain where it is to ensure the popover appears and is above other block elements in the stacking order. A tabindex of -1 ensures the hint cannot take keyboard focus.

@sebelga
Copy link
Contributor Author

sebelga commented Oct 25, 2022

Thanks for the review @constancecchen and @1Copenut I addressed your feedback. Can you have another look? Thanks!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6319/

@1Copenut
Copy link
Contributor

Thanks for the review @constancecchen and @1Copenut I addressed your feedback. Can you have another look? Thanks!

Sure thing. I pulled latest changes at 9:00 am Central Time on 25 October. I have two more changes to request for accessibility. Adding two screen shots below to hopefully illustrate those points. Let me know if you want to review together or have questions about the change requests.


Request 1: Move the unique ID to EuiPopoverPanel

Screen Shot 2022-10-25 at 9 32 48 AM


Request 2: Remove the tabindex or set it to -1"

Screen Shot 2022-10-25 at 9 33 05 AM

@sebelga
Copy link
Contributor Author

sebelga commented Oct 25, 2022

@1Copenut I am not sure I follow what you mean.

I am consuming an already existing EUI component. If I am understanding you correctly you are suggesting to make changes to that component. I would prefer to create an issue for this and fix what you saw separately.

Unless there is something in the code that I added you want me to change? Can you indicate me the change? Cheers.

@sebelga
Copy link
Contributor Author

sebelga commented Oct 27, 2022

Thanks for clarifying and sorry for misreading tabIndex for z-index!
I fixed the issue in 7fa168b

Screenshot 2022-10-27 at 11 23 02

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6319/

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Code LGTM - just 1 optional cleanup comment. Screen reader QA worked great as well (tested in FF/VO), although I'd definitely appreciate another pass from you Trevor!

src/components/search_bar/search_bar.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Please see my one requested change, no additional review needed from me. If you want to verify the change, download the axe browser plugin and run it on the page with a hint open. Should be no a11y errors in the plugin.

Thank you for the second change set @sebelga!

src/components/search_bar/search_box.tsx Outdated Show resolved Hide resolved
@sebelga
Copy link
Contributor Author

sebelga commented Oct 28, 2022

Thanks for the review! 👍 I addressed the latest feedback in 1205818

@sebelga sebelga enabled auto-merge (squash) October 28, 2022 12:59
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6319/

@sebelga sebelga merged commit 0267215 into elastic:main Oct 28, 2022
@cee-chen
Copy link
Member

Thanks for the great contributions @sebelga!! 🎉

@chandlerprall chandlerprall mentioned this pull request Nov 1, 2022
8 tasks
jbudz pushed a commit to elastic/kibana that referenced this pull request Nov 18, 2022
## Summary
`eui@67.1.8` ⏩ `eui@70.2.2`

⚠️ Note: This upgrade contains breaking changes to `EuiFlexGroup` and
`EuiFlexGrid`, primarily around switching margins and negative margins
to `gap`. Please do a quick QA pass of your app to scan for any issues.
We're happy to help resolve minor fixes, or potentially follow up after
PR merges. You can find us over in #eui!

## [`70.2.4`](https://github.com/elastic/eui/tree/v70.2.4)

**Bug fixes**

- Fixed visual bug in nested `EuiFlexGroup`s, where the parent
`EuiFlexGroup` is responsive but a child `EuiFlexGroup` is not
([#6381](elastic/eui#6381))

## [`70.2.3`](https://github.com/elastic/eui/tree/v70.2.3)

**Bug fixes**

- Fixed incorrect margins in `EuiSuperDatePicker` caused by `EuiFlex`
CSS gap change ([#6380](elastic/eui#6380))

## [`70.2.2`](https://github.com/elastic/eui/tree/v70.2.2)

- `EuiButton` now accepts `minWidth={false}`
([#6373](elastic/eui#6373))

**Bug fixes**

- `EuiButton` no longer outputs unnecessary inline styles for
`minWidth={0}` or `minWidth={false}`
([#6373](elastic/eui#6373))
- `EuiFacetButton` no longer reports type issues when passing props
accepted by `EuiButton`
([#6373](elastic/eui#6373))
- Fixed the shadow sizes of `.eui-yScrollWithShadows` and
`.eui-xScrollWithShadows`
([#6374](elastic/eui#6374))


## [`70.2.1`](https://github.com/elastic/eui/tree/v70.2.1)

**Bug fixes**

- Re-fixed `EuiPageSection` not correctly merging `contentProps.css`
([#6365](elastic/eui#6365))
- Fixed `EuiTab` not defaulting to size `m`
([#6366](elastic/eui#6366))

## [`70.2.0`](https://github.com/elastic/eui/tree/v70.2.0)

- Added a keyboard shortcuts popover to `EuiDataGrid`'s toolbar. This
can be visually hidden via `toolbarVisibility.showKeyboardShortcuts`,
but will always remain accessible to keyboard and screen reader users.
([#6036](elastic/eui#6036))
- `EuiScreenReaderOnly`'s `showOnFocus` prop now also shows on focus
within its children ([#6036](elastic/eui#6036))
- Added `onFocus` prop callback to `EuiSuperDatePicker`
([#6320](elastic/eui#6320))

**Bug fixes**

- Fixed `EuiSelectable` to ensure the full options list is re-displayed
when the search bar is controlled and cleared using `searchProps.value`
([#6317](elastic/eui#6317))
- Fixed incorrect padding on `xl`-sized `EuiTabs`
([#6336](elastic/eui#6336))
- Fixed `EuiCard` not correctly merging `css` on its child `icon`s
([#6341](elastic/eui#6341))
- Fixed `EuiCheckableCard` not setting `css` on the correct DOM node
([#6341](elastic/eui#6341))
- Fixed a webkit rendering issue with `EuiModal`s containing
`EuiBasicTable`s tall enough to scroll
([#6343](elastic/eui#6343))
- Fixed bug in `to_initials` that truncates custom initials
([#6346](elastic/eui#6346))
- Fix bug in `EuiCard` where layout breaks when `horizontal` and
`selectable` are both passed
([#6348](elastic/eui#6348))

## [`70.1.0`](https://github.com/elastic/eui/tree/v70.1.0)

- Added the `hint` prop to the `<EuiSearchBar />`. This prop lets the
consumer render a hint below the search bar that will be displayed on
focus. ([#6319](elastic/eui#6319))
- Added the `hasDragDrop` prop to `EuiPopover`. Use this prop if your
popover contains `EuiDragDropContext`.
([#6329](elastic/eui#6329))

**Bug fixes**

- Fixed `EuiButton`'s cursor style when the button is disabled
([#6323](elastic/eui#6323))
- Fixed `EuiPageTemplate` not recognizing child
`EuiPageSidebar`s/`EuiPageTemplate.Sidebar`s with `css` props
([#6324](elastic/eui#6324))
- Fixed `EuiBetaBadge` to always respect its `anchorProps` values,
including when there is no tooltip content
([#6326](elastic/eui#6326))
- Temporarily patched `EuiModal` to not cause scroll-jumping issues on
modal open ([#6327](elastic/eui#6327))
- Fixed buggy drag & drop behavior within `EuiDataGrid`'s columns &
sorting toolbar popovers
([#6329](elastic/eui#6329))
- Fixed `EuiButton` not correctly passing `textProps` for children
inside fragments or i18n components
([#6332](elastic/eui#6332))
- Fixed `EuiButton` not correctly respecting `minWidth={0}`
([#6332](elastic/eui#6332))

**CSS-in-JS conversions**

- Converted `EuiTabs` to Emotion
([#6311](elastic/eui#6311))

## [`70.0.0`](https://github.com/elastic/eui/tree/v70.0.0)

- Added the `enabled` option to the `<EuiInMemoryTable />`
`executeQueryOptions` prop. This option prevents the Query from being
executed when controlled by the consumer.
([#6284](elastic/eui#6284))

**Bug fixes**

- Fixed `EuiOverlayMask` to set a
`[data-relative-to-header=above|below]` attribute to replace the
`--aboveHeader` and `--belowHeader` classNames removed in its Emotion
conversion ([#6289](elastic/eui#6289))
- Fixed `EuiHeader` CSS using removed `EuiOverlayMask` class modifiers
([#6293](elastic/eui#6293))
- Fixed `EuiToolTip` not respecting reduced motion preferences
([#6295](elastic/eui#6295))
- Fixed a bug with `EuiTour` where passing any `panelProps` would cause
the beacon to disappear
([#6298](elastic/eui#6298))

**Breaking changes**

- `@emotion/css` is now a required peer dependency, alongside
`@emotion/react` ([#6288](elastic/eui#6288))
- `@emotion/cache` is no longer required peer dependency, although your
project must still use it if setting custom cache/injection locations
([#6288](elastic/eui#6288))

**CSS-in-JS conversions**

- Converted `EuiCode` and `EuiCodeBlock` to Emotion; Removed
`euiCodeSyntaxTokens` Sass mixin and `$euiCodeBlockPaddingModifiers`;
([#6263](elastic/eui#6263))
- Converted `EuiResizableContainer` and `EuiResizablePanel` to Emotion
([#6287](elastic/eui#6287))

## [`69.0.0`](https://github.com/elastic/eui/tree/v69.0.0)

- Added support for `fullWidth` prop on EuiForm, which will be the
default for all rows/controls within
([#6229](elastic/eui#6229))
- Added support for `onResizeStart` and `onResizeEnd` callbacks to
`EuiResizableContainer`
([#6236](elastic/eui#6236))
- Added optional case sensitive option matching to `EuiComboBox` with
the `isCaseSensitive` prop
([#6268](elastic/eui#6268))
- `EuiFlexItem` now supports `grow={0}`
([#6270](elastic/eui#6270))
- Added the `alignItems` prop to `EuiFlexGrid`
([#6281](elastic/eui#6281))
- Added `filter`, `filterExclude`, `filterIgnore`, `filterInclude`,
`indexTemporary`, `infinity`, `sortAscending`, and `sortDescending`
glyphs to `EuiIcon` ([#6282](elastic/eui#6282))

**Bug fixes**

- Fixed `EuiTextProps` to show the `color` type option `inherit` as
default ([#6267](elastic/eui#6267))
- `EuiFlexGroup` now correctly respects `gutterSize` when responsive
([#6270](elastic/eui#6270))
- Fixed the last breadcrumb in `EuiBreadcrumbs`'s `breadcrumbs` array
not respecting `truncate` overrides
([#6280](elastic/eui#6280))

**Breaking changes**

- `EuiFlexGrid` no longer supports `columns={0}`. Use `EuiFlexGroup`
instead for normal flex display
([#6270](elastic/eui#6270))
- `EuiFlexGrid` now uses modern `display: grid` CSS
([#6270](elastic/eui#6270))
- `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` now use modern `gap`
CSS instead of margins and negative margins
([#6270](elastic/eui#6270))
- `EuiFlexGroup` no longer applies responsive styles to `column` or
`columnReverse` directions
([#6270](elastic/eui#6270))

**CSS-in-JS conversions**

- Converted `EuiFlexGroup`, `EuiFlexGrid`, and `EuiFlexItem` to Emotion
([#6270](elastic/eui#6270))

## [`68.0.0`](https://github.com/elastic/eui/tree/v68.0.0)

- Added `beta` glyph to `EuiIcon`
([#6250](elastic/eui#6250))
- Added `launch` and `spaces` glyphs to `EuiIcon`
([#6260](elastic/eui#6260))
- Added the `fallbackDestination` prop to `EuiSkipLink`, which accepts a
string of query selectors to fall back to if the `destinationId` does
not have a valid target. Defaults to `main`
([#6261](elastic/eui#6261))
- `EuiSkipLink` is now always an `a` tag to ensure that it is always
placed within screen reader link menus.
([#6261](elastic/eui#6261))

**Bug fixes**

- Fixed `EuiSuperDatePicker` not correctly merging passed `className`s
([#6253](elastic/eui#6253))
- Fixed `EuiColorStops` not correctly merging in passed
`data-test-subj`s, `style`s, or `...rest`
([#6255](elastic/eui#6255))
- Fixed `EuiResizablePanel` incorrectly passing `style` to the wrapper
instead of the panel. Use `wrapperProps.style` to pass styles to the
wrapper. ([#6255](elastic/eui#6255))
- Fixed custom `onClick`s passed to `EuiSkipLink` overriding
`overrideLinkBehavior`
([#6261](elastic/eui#6261))

**Breaking changes**

- Removed `inherit` and `ghost` color from `EuiListGroupItem`
([#6207](elastic/eui#6207))
- Changed default color to `text` instead of `inherit`
([#6207](elastic/eui#6207))

**CSS-in-JS conversions**

- Converted `EuiListGroup` and `EuiListGroupItem` to Emotion; Removed
`$euiListGroupGutterTypes`, `$euiListGroupItemColorTypes` and
`$euiListGroupItemSizeTypes`;
([#6207](elastic/eui#6207))
- Converted `EuiBadgeGroup` to Emotion
([#6258](elastic/eui#6258))
- Converted `EuiBetaBadge` to Emotion
([#6258](elastic/eui#6258))
- Converted `EuiNotificationBadge` to Emotion
([#6258](elastic/eui#6258))

Co-authored-by: Elizabet Oliveira <elizabet.oliveira@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
edmarmoretti pushed a commit to edmarmoretti/eui that referenced this pull request May 3, 2023
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.

[EuiSearchBar] Add the option to render a hint below the bar
5 participants