[EuiDataGrid] Add keyboard shortcuts reference #6036

cee-chen · 2022-07-07T21:39:49Z

Summary

closes #2482, see also elastic/kibana#90515 for reference

This PR adds a keyboard shortcuts popover to the EuiDataGrid toolbar. The primary target audience of this functionality is non-sighted screen reader users who otherwise have no indication of how to interact with the data grid.

Checklist

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in Chrome, Safari, Edge, and Firefox
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~

Checked in mobile
Added documentation
Props have proper autodocs and playground toggles
Added or updated jest and cypress tests
Updated the Figma library counterpart
A changelog entry exists and is marked appropriately

1Copenut · 2022-07-07T21:50:54Z

Tagging along as a reviewer. Linking to the wider Kibana issue discussing global keyboard shortcuts for visibility. I think we can use this as a starting point for consistent UX and to road test a11y.

kibanamachine · 2022-07-07T22:07:16Z

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

[CONSTANCE EDIT] Moving my original PR description to this comment to maintain conversation clarity

⚠️ WIP, opening for early staging QA/testing and feedback

@miukimiu Just opening this for a quick reference - I'd love to get your thoughts on the design, styling, etc:

Is this too large for a popover? Would a modal be better?
Does an EuiDescriptionList work best for this? Or would, e.g. a table or similar be better?
I don't love the "info" icon but I don't think the "keyboardShortcut" icon is much better, it looks a bit confusing. Is there any chance we could add a keyboard icon that looks maybe something like this?

miukimiu · 2022-07-12T14:40:49Z

Hi @constancecchen 👋🏽 ,

I think the popover works.
A basic table works well.
There was a discussion about the keyboardShortcut icon (Add keyboard shortcut icon #1573). And there were some attempts to add a keyboard instead of just keys and it didn't work out. So I think is better if we stick with the current keyboardShortcut.

For the keyboard shortcut what do you think of using the tag <kbd> instead of a <EuiBadge>? But I think we need better styles for the <kbd> tag which relates to this issue: #5016. I can prioritize this issue and add a new component.

I did some tests in a CodeSandbox and with what I suggested the popover would look like the following screenshot:

CodeSandbox design idea:
https://codesandbox.io/s/modest-jepsen-6xsj0v?file=/demo.js

Note
The kbd styles might change a little. But the styles were based on what we have in Figma: https://www.figma.com/file/RzfYLj2xmH9K7gQtbSKygn/Elastic-UI?node-id=13648%3A4134

cee-chen · 2022-07-12T19:37:57Z

Love it, thanks Elizabet! <kbd> definitely makes more sense. One quick note, I don't think the table needs to have a responsive mode as the popover is already very small. What do you think?

Re the keyboardShortcut icon, we chatted about this in sync today and it sounds like we may be revisiting it - I can use it for now and we can contemplate updating it in-place later.

miukimiu · 2022-07-14T11:34:37Z

One quick note, I don't think the table needs to have a responsive mode as the popover is already very small. What do you think?

@constancecchen I agree. The table doesn't need the responsive mode. I updated the CodeSandbox.

miukimiu · 2022-07-14T20:07:26Z

New icon

@constancecchen here are a few ideas for the new icon:

I'm not 100% convinced these icons work in 16px x 16px but let me hear your opinion.

The `iInCircle` icon

Another option would be to keep your initial idea of using the iInCircle icon. I think it works better than the old keyboardShortcut icon. Also, it aligns with other "product's help" across Kibana and Cloud as you can see in our In product help guidelines.

Just to show an example in Discover:

Prototype with the `iInCircle` and new icon

Here are a few mocks with both iInCircle and the new icon in the toolbar or footer: Figma prototype.

In the prototype, one of the ideas is to keep the icon in the toolbar.

Another idea would be to align with the markdown editor and keep the "help" in the footer:

Let me hear your thoughts.

cee-chen · 2022-07-18T15:56:54Z

I love it - I like the 6th keyboard icon the most personally! 🎉

I'd push against putting the keyboard shortcuts button on the bottom bar / next to the pagination because of how that affects screen reader users - they won't even get to that by the time they've finished interacting with the grid.

EDIT: Also worth mentioning, I'm not strongly against using the help icon, but I think I'd prefer a keyboard icon for EuiDataGrid specifically. This is for several reasons:

It really does only contain keyboard shortcuts, unlike the Kibana examples where the popover is explaining other things
EuiDataGrid has a specific need for defining keyboard shortcuts: normal screen reader and keyboard navigation (e.g. tab, ctrl+opt+arrow keys) does not work as normal / as expected. These keyboard shortcuts are not optional enhancements on top of existing behavior; it is necessary that we note that there is a specific way of interacting with EuiDataGrid, or we risk SR/keyboard users not knowing what to do.
We're trying to establish a pattern for Kibana to use for defining keyboard shortcuts in the future (Keyboard shortcut registry kibana#90515), and I think it makes sense that there will be more keyboard shortcut popovers in the future in Kibana

cee-chen · 2022-07-18T19:56:47Z

@1Copenut Hey Trevor! Before I switch this branch over to using a table layout format for the keyboard shortcuts, could I get your opinion on the screen reader experience of it?

<table>: https://codesandbox.io/s/modest-jepsen-6xsj0v?file=/demo.js
<dl>/<dt>/<dd>: https://eui.elastic.co/pr_6036/#/tabular-content/data-grid

I could definitely be wrong here, but I'm surprisingly not a super huge fan of the table experience. I think the screen reader ends up reading a lot of cruft like 'row X of Y' and mashes the column title + cell contents together and it leads to a more distracting experience with that information than without (the description list just reads out a '20 of 24' at the end of the item).

I could be totally wrong about that not being useful info though - WDYT?

kibanamachine · 2022-07-18T20:15:43Z

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

1Copenut · 2022-07-19T20:47:18Z

@constancecchen I'm leaning toward this as a description list too. It's subtle, but my thinking is that the description list is used to define key / value pairs. I feel our intended audience for the aural readback will be familiar with what each term means, so having table columns read each time could be extraneous.

The only thing I might suggest adding would be an aria-labelledby="HEADING_ID" to the DL tag that references a heading in the popover. That gives the list a unique name in SR Lists menus, and reinforces the relationship between the heading as proxy for table columns.

1Copenut

Just two comments here @constancecchen. LMK if you want to have questions or want to discuss further.

src/components/datagrid/controls/keyboard_shortcuts.tsx

1Copenut · 2022-07-19T20:59:33Z

src/components/datagrid/controls/keyboard_shortcuts.tsx

+              {
+                title: (
+                  <>
+                    <kbd>Ctrl</kbd> <kbd>Home</kbd>


What do you think about adding a + between the keys for these commands that require 2 keys together in a chord?

Hahaha, we had this discussion in #6049 as well. Apparently I'm the only one who hates the +s 🙈

For what it's worth, it looks like there isn't a standard for this online that I can tell. From Michail's keyboard shortcuts research in elastic/kibana#90515, it looks like Google uses + between combos and GitHub doesn't. Obviously I'm biased towards GitHub in this case 🤪

FWIW, I think I dislike the + because of how I personally say keyboard combos: when I both say or read combos out loud, I say "control c" all together or "control alt delete" for example, I don't actually say "control plus c" or "control plus v".

Let's go with your way to not have the + between.

It's a new pattern so there's room for interpretation. Your example about how you read and say key combos made me chuckle; I tend to say them the same way but see them visually with the plus sign as a reinforcement that this is a chord and not a sequence.

cee-chen · 2022-07-19T21:10:25Z

Awesome, thank you @1Copenut! 🙌

@miukimiu Are you OK with moving forward with using EuiDescriptionList instead of EuiBasicTable? It changes the visual appearance somewhat, but if you want I can try to get back to that visual appearance w/ CSS alone rather than a table structure. For what it's worth, I feel fairly strongly about this as the biggest beneficiaries of the keyboard shortcuts panel is keyboard/screen reader users 😅

EDIT: any custom CSS tweaking should perhaps wait until Bree is done converting EuiDescriptionList in #5971. This PR can definitely hang around until then, there's no rush!

kibanamachine · 2022-07-19T21:53:55Z

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

miukimiu · 2022-07-20T16:28:49Z

@miukimiu Are you OK with moving forward with using EuiDescriptionList instead of EuiBasicTable?

Yes, I'm ok with that. I'll update CodeSandbox to reflect that.

cchaos · 2022-07-25T17:38:56Z

I'd like to make one quick suggestion on the formatting of the popover contents in an effort to make it more concise. Here's a screenshot:

Use the emoticon where possible to signify the correct key. It's much easier to scan for ↓ than arrow down.
Combine some of the simple & similar shortcuts in a comma separated list
Use bold format to highlight the key differences in the command
Try not to use terms like "popover" but instead "details"

Also, I'd label this "keyboard navigation" vs "shortcuts" because "shortcuts" assumes there's a menu item associated with the command. But really, this is just using the keyboard to traverse the elements.

If we want to think scalability, I'd probably opt for the iInCircle icon as the triggering button so that this popover could then contain more information in the future.

One item I'm still not clear on is the page up/down navigation. I can't tell if something is broken or why it's not another "focus" type of navigation.

cee-chen · 2022-07-25T22:13:30Z

Use the emoticon where possible to signify the correct key. It's much easier to scan for ↓ than arrow down.

I'm super digging this! Screen readers are reading the arrows out perfectly as well (although a quick note that we do need to use the actual unicode arrow characters vs emojis)

Combine some of the simple & similar shortcuts in a comma separated list

I'm not 100% confident about this but I strongly worry about the cognitive separation here for screen reader users, who will be navigating through 4 different arrow keys and then just "moves cell focus".

For what it's worth, when I separated each individual arrow key description, I was following W3's pattern of listing shortcuts: https://www.w3.org/WAI/ARIA/apg/patterns/grid/#keyboard-interaction-for-data-grids

Sadly W3 doesn't have a pattern/spec for listing keyboard shortcuts (https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface is fairly vague / doesn't mention how to tell consumers what your keyboard shortcuts are), so I don't have a lot of other examples/standards to reference, but as another example GitHub also separates out each arrow key: https://docs.github.com/en/get-started/using-github/keyboard-shortcuts#network-graph

@1Copenut, any thoughts?

Try not to use terms like "popover" but instead "details"

Sure, but for SR users we should somehow inform them that not all cells have details / are interactive (in case they press enter on a non-interactive cell and get nothing). It's probably also worth mentioning that cell actions are available in the details (because cell actions are not available or even mentioned to non-sighted keyboard users except through the interactive cell popover).

If you really want to be brief for sighted users, we could take advantage of SR-only text to do something like:

Opens cell details <EuiScreenReaderOnly>for interactive cells that contain cell actions or overflow data. These details open in a new dialog.</EuiScreenReaderOnly>.

Also, I'd label this "keyboard navigation" vs "shortcuts" because "shortcuts" assumes there's a menu item associated with the command. But really, this is just using the keyboard to traverse the elements.

Looks like https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface agrees with you - it primarily references keyboard shortcuts in terms of 'assigning and revealing' and otherwise uses 'keyboard navigation'.

Honestly though, I think that line is pretty blurry (e.g. looking at GitHub shortcuts again, many of these are navigation adjacent - although of course many aren't).

Re: page up/down navigation - it should be focusing the same cell column but in the last/next page's last/first row. Unfortunately W3 has no examples of data grids with pagination so we're deviating from their spec there / making up our own rules for page up/down.

If we want to think scalability, I'd probably opt for the iInCircle icon as the triggering button so that this popover could then contain more information in the future.

I would strongly rather keep the focus of this popover on keyboard accessibility. As I mentioned earlier, screen reader and keyboard users unfamiliar with the spec and standard keyboard interaction for data grids will potentially either not know or be unable to fully interact with the grid otherwise. The standard SR way of browsing through items will fail them for grids that scroll due to the virtualization library, as our grid only scrolls to cells out of view via arrow keys and not via SR arrow navigation.

It's also worth noting the custom logic I added to this button, while consumers can opt to 'hide' this button via toolbarVisibility.showKeyboardShortcuts: false, it only visually hides the button - keyboard users who tab in sequence through the data grid toolbar will always have these keyboard shortcuts available to them to view. I don't think this paradigm is necessary for other random information.

My goal with this PR was to establish a pattern that Kibana could reference or use in elastic/kibana#90515 and get ahead of a few issues (such as a few we've already solved, e.g. should we use a table or a description list? is the icon clearly readable as a keyboard?). While it's not 1:1 because EuiDataGrid doesn't exactly have 'shortcuts' or assigning shortcuts as you mentioned Caroline, nevertheless I still think it's worth keeping keyboard documentation as a first class citizen.

1Copenut · 2022-07-26T18:44:25Z

Combine some of the simple & similar shortcuts in a comma separated list

I'd opt to leave the list as it is now, with each navigation key / chord having its own definition. It's verbose but there's a clear relationship between key and value in each pair.

Also, I'd label this "keyboard navigation" vs "shortcuts" because "shortcuts" assumes there's a menu item associated with the command. But really, this is just using the keyboard to traverse the elements.

I concur this feels like navigation. If we use this nomenclature for data grids, we can extend and use "shortcuts" when we expand this UI into Kibana custom commands. At that point it really is a shortcut; we'd be removing steps between A and B.

If we want to think scalability, I'd probably opt for the iInCircle icon as the triggering button so that this popover could then contain more information in the future.

I second keeping the focus of this popover and its icon on keyboard navigation. The navigation pattern we're looking to establish is specifically for data grids and is wholly driven by key presses. Let's keep the ilnCircle for a Yes And when we start introducing keyboard shortcuts in Kibana.

cchaos · 2022-07-26T20:14:56Z

It's a compromise between re-stating obvious/common navigational patterns and highlighting those that are unique. The cell expansion ones are the most unique but are at the bottom of the list most likely hidden by scroll if every item is listed by itself. We don't have to be quite as brief as Moves cell focus, but could be Moves cell focus up, down, left, right respectively.

I only mention the scalability because the toolbars are getting quite cramped and most of the time these keyboard commands charts are a one-time necessity, yet it'll be displayed on all (possibly multiple on one page) data grids all the time. I'm not saying they're not helpful but I'd be careful how close to and combined with actual data grid controls.

github-actions · 2022-10-25T00:06:11Z

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

- which expands/makes the prop more flexible for, e.g. buttons wrapped in tooltips or popovers

- if set to false, visually hide but still allow keyboard users to focus/access the info

- Move keyboard shortcuts button to leftmost position in icon controls - Add EuiPopoverTitle

kibanamachine · 2022-10-27T20:01:18Z

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

kibanamachine · 2022-10-27T21:37:12Z

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

miukimiu · 2022-10-28T11:48:37Z

@florent-leborgne can you help us with this PR?

This PR adds a keyboard shortcuts popover to the EuiDataGrid toolbar. The primary target audience of this functionality is low visual screen reader users who otherwise have no indication of how to interact with the data grid.

This is the current implementation that can be found here. To open the popover you just need to click the keyboard icon of the data grid:

Our questions are:

1. Should we be using the word "popover" or "details"? For example:

"Opens cell expansion popover for interactive cells" or "Open cell details"
"Closes any open popovers." or "Close cell details/Exits full screen"

2. Should the title of the popover be "Keyboard shortcuts" or "Keyboard navigation"?
3. Should we end each line with a dot?

Also, feel free to suggest other text changes.

florent-leborgne · 2022-10-28T13:38:44Z

Thanks @miukimiu for pinging me.

Should we be using the word "popover" or "details"? For example:

"Opens cell expansion popover for interactive cells" or "Open cell details"
"Closes any open popovers." or "Close cell details/Exits full screen"

I think details is much simpler. "Open/Close cell details" feels very simple in comparison to the full technical description of what really happens.

Should the title of the popover be "Keyboard shortcuts" or "Keyboard navigation"?

Keyboard shortcuts is fine. Small (maybe unrelated) question though: Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

Should we end each line with a dot?

I think no punctuation is fine. And no 3rd person needed as well. Here is what I came up with, with these changes and a few other minor tweaks:

Let me know what you think, I'm happy to iterate.

miukimiu · 2022-10-28T13:51:59Z

Thanks, @florent-leborgne for being so fast. All the changes look good to me. @constancecchen do you agree with the changes?

Small (maybe unrelated) question though: Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

I pass this question to @constancecchen. She's implementing the code.

florent-leborgne · 2022-10-28T14:36:00Z

Just for reference there are other shortcut sections in some places of the products (see screenshot as example).
Would this become the new standard for all shortcut references?

miukimiu · 2022-10-28T14:54:57Z

Just for reference there are other shortcut sections in some places of the products (see screenshot as example).
Would this become the new standard for all shortcut references?

@florent-leborgne we would like to enforce the usage of the EuiText <kbd> HTML element that was introduced in EUI v61.0.0. This element is used to indicate text that represents keyboard input from a user’s computer.

This would ensure all shortcuts would visually look similar
Also a better semantic usage

Would this information be a good candidate to be part of "in product assistance guidelines"?

florent-leborgne · 2022-10-28T15:08:00Z

Would this information be a good candidate to be part of "in product assistance guidelines"?

@miukimiu Yes totally!

florent-leborgne · 2022-10-28T15:28:52Z

One last suggestion on the text (thanks @gchaps); How about using bold format to highlight the key differences (like Caroline suggested earlier in this PR). I think it would help scanning the list a bit faster.

cee-chen · 2022-10-28T16:37:10Z

I apologize in advance if I appear argumentative, it's totally not my intention. But I want to stress a few things with this PR:

The goal of this PR is accessibility for non-sighted users. While visual/mouse users benefit from knowing keyboard shortcuts, they don't need this information in the same way that non-sighted users do. Screen reader users literally have no other guidance than this popover on how to correctly interact with the data grid, as the normal method of screen reader navigation in EuiDataGrid does not really work.
With that context in mind, while I appreciate the desire for brevity, I strongly think we need to err on the side of being explicit. Basically my litmus is, "if a blind user would not know otherwise know this information, do we need to spell out? Yes."
I want to mention that the majority of this copy I originally cribbed from W3 itself. W3C essentially sets the standard for accessibility and accessibility examples.

With that context in mind, I wanted to answer a few questions:

"Opens cell expansion popover for interactive cells" or "Open cell details" [...] I think details is much simpler. "Open/Close cell details" feels very simple in comparison to the full technical description of what really happens.

One minor objection - the cell expansion popover does not necessarily just contain more details. It also contains any truncated/overflowing cell actions that keyboard users literally cannot access outside of the expansion popover. The cell content itself is secondary to the fact that cell actions (i.e., interactive cells) must be accessed by keyboard user via popover, hence the stress on 'interactive cells'.

That being said, I get that it's wordy, and now that I write it out my main issue with the simplification, I think we can find a compromise. "Open cell details & actions" might sufficiently cover what I'm trying to convey 😅 WDYT? Any suggestions?

Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

For this I'm following W3's example which is only listing "Control". I think that might be because 90%~ of screen reader users are on Windows however (per WebAIM's 2021 survey). @1Copenut, any thoughts on this? Should we try to write a component that detects the user OS and displays Cmd instead?

(Ctrl) Home/End copy suggestions

"end/beginning of the row" is not enough information for a screen reader keyboard user. IMO, they need to know that it's the first cell/beginning of the row of the cell they are currently focused on (i.e., the current row). I don't think it's implied which row, otherwise. I also really think the ctrl descriptions are slightly misleading as it's not the first/last cell of the entire table (which may be paginated) - it's the first cell of the first row of the current page, and last cell of the last row of the last page.

I also personally prefer keeping the 'cell' copy between descriptions to make it clear that keyboard navigation centers around cells, but maybe that's just me.

How about using bold format to highlight the key differences (like Caroline suggested earlier in this PR). I think it would help scanning the list a bit faster.

Unfortunately this makes i18n translation a fairly annoying headache and I'm not confident it will translate well between languages. I would rather leave this alone (while screen readers do pick up emphasis, I'm not really sure that would provide all that much context to them). I don't feel very strongly about this however so am open to pushback if we really feel bolding is that useful.

1Copenut · 2022-10-28T17:49:52Z

Great discussion! I agree 100% with @constancecchen that explicit, clear explanations are our goal for this shortcut reference. This sets us up well for future shortcut menus when there's questions about clarity vs. brevity. I almost always come down on the side of verbosity and explicit instruction for assistive technology. Instructions that are present are easy to skip, but implied instructions are sometimes hard to deduce.

I'm quote answering the two items from comments above that I can add some insight.

That being said, I get that it's wordy, and now that I write it out my main issue with the simplification, I think we can find a compromise. "Open cell details & actions" might sufficiently cover what I'm trying to convey 😅 WDYT? Any suggestions?

This feels like a good compromise for clarity with brevity. If I was reading this with nothing else to go on, I could infer the cell had more information (details) and possibly interactions I could drive (actions).

Those are for Windows (ctrl + home). Do you plan on displaying equivalent Mac shortcuts as well (usually fn+arrows for more advanced moves)?

We won't need to add alternatives for the Ctrl + Home and Ctrl + End chords. Safari + VO on MacOS uses the Control key to activate these controls. Pressing Cmd + Home takes you to the Apple website by default in Safari.

florent-leborgne · 2022-10-31T10:21:24Z

Thank you so much @constancecchen and @1Copenut for the insights and background!

Open cell details & actions

This sounds good to me, maybe "Open cell details and actions", with plain text instead of &?

Safari + VO on MacOS uses the Control key to activate these controls.

Ok thank you! There are no obvious Home and End keys on my macbook so I had to Google for equivalents and I felt this could cause confusion.

the cell they are currently focused on (i.e., the current row). I don't think it's implied which row, otherwise.

All shortcuts are about moving from the current position so I really thought it would be OK to not repeat "current something" every time. I think we have 3 choices here: add it everywhere (I think it wouldn't help scanning), add it nowhere and consider it implied, or maybe add a small sentence at the top saying something like "You can navigate cells and change focus from its current position using the following shortcuts.".

I very much agree about your other points so I can make these new suggestions. Feel free to adapt them or use the ones from W3 as you ultimately have more background than me about best practices for accessibility:

Move to the beginning of the row: Move to the first cell of the row
Move to the end of the row: Move to the last cell of the row
Move to the first cell of the table: Move to the first cell of the active page
Move to the last cell of the table: Move to the last cell of the active page

Ultimately, if you feel like my suggestions strip out too much technical accuracy, we can go for the descriptions from W3, but I think that at least some parts are unnecessarily verbose compared to established grid/table navigation practices, and are even sometimes unclear if we go back to the point you made about "current focus" in your explanations. (for example, moves focus to the first cell in the first row. on W3 doesn't say if it's the first row from the current page either).

I would rather leave this alone (about bold)

This is a nice addition for many users even if they are not the primary target audience of this component. Nice doesn't mean critical as the list is still fairly short so I'll leave it up to you :)

Thanks again for all the great points!

cee-chen · 2022-10-31T22:17:42Z

@florent-leborgne does this current iteration of copy work for you? I personally prefer the specificity of including current as an adjective but would be open to changing it to something like active instead if you think it sounds better!

florent-leborgne · 2022-11-02T09:40:08Z

@constancecchen Your last suggestion looks good to me :) Thanks!

cee-chen · 2022-11-02T15:56:01Z

Thanks a million y'all!! I really appreciate all the discussion and guidance around this feature! @1Copenut and @miukimiu, do you mind doing a final approval pass?

kibanamachine · 2022-11-02T16:25:01Z

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

miukimiu

Thanks, @constancecchen.

I tested in Chrome, Safari, Edge, and Firefox. I also looked at the code.

LGTM! 👍🏽

cee-chen · 2022-11-03T16:49:11Z

Thanks @miukimiu! Just to check, can I check "updated Figma library" off the PR checklist?

miukimiu · 2022-11-03T17:12:59Z

@constancecchen, yes! 👍🏽

cee-chen · 2022-11-03T17:23:51Z

cee-chen · 2022-11-07T16:26:31Z

@1Copenut Still waiting on your QA/review for this!

1Copenut

👍 LGTM! I tested the keyboard navigation for focus, focus trap, and ESC to close the helper popover. Also tested with VoiceOver on Safari and Firefox to ensure the keyboard shortcut dialog and shortcuts were communicated clearly.

cee-chen · 2022-11-07T22:55:52Z

Woohoo, merging!! Thanks everyone!

## 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>

cee-chen requested a review from miukimiu July 7, 2022 21:39

1Copenut self-requested a review July 7, 2022 21:48

miukimiu mentioned this pull request Jul 18, 2022

[EuiIcon] Add keyboard glyph #6058

Merged

11 tasks

1Copenut reviewed Jul 19, 2022

View reviewed changes

github-actions bot added the stale-pr label Oct 25, 2022

1Copenut removed the stale-pr label Oct 25, 2022

cee-chen added 6 commits October 27, 2022 11:20

Add :focus-within support to EuiScreenReaderOnly showOnFocus

5152269

- which expands/makes the prop more flexible for, e.g. buttons wrapped in tooltips or popovers

Set up keyboard shortcuts button + popover + styling

1f45809

Set up showKeyboardShortcuts configuration

8bbaac1

- if set to false, visually hide but still allow keyboard users to focus/access the info

Add documentation

380b4b4

Update PR with recently added new keyboard icon and kbd styles

8cf2c1f

[PR feedback] Misc positioning

ab66280

- Move keyboard shortcuts button to leftmost position in icon controls - Add EuiPopoverTitle

Update/fix datagrid E2E tests

131408e

[PR feedback] Copy

2fed236

cee-chen requested a review from 1Copenut November 2, 2022 15:56

miukimiu approved these changes Nov 2, 2022

View reviewed changes

1Copenut approved these changes Nov 7, 2022

View reviewed changes

cee-chen merged commit a4493d9 into elastic:main Nov 7, 2022

cee-chen deleted the datagrid/keyboard-shortcuts branch November 7, 2022 22:55

cee-chen mentioned this pull request Nov 8, 2022

Upgrade EUI to v70.2.4 elastic/kibana#144845

Merged

[EuiDataGrid] Add keyboard shortcuts reference #6036

[EuiDataGrid] Add keyboard shortcuts reference #6036

Conversation

cee-chen commented Jul 7, 2022 • edited Loading

Summary

Checklist

1Copenut commented Jul 7, 2022

kibanamachine commented Jul 7, 2022 • edited by cee-chen Loading

miukimiu commented Jul 12, 2022

cee-chen commented Jul 12, 2022

miukimiu commented Jul 14, 2022

miukimiu commented Jul 14, 2022

New icon

The iInCircle icon

Prototype with the iInCircle and new icon

cee-chen commented Jul 18, 2022 • edited Loading

cee-chen commented Jul 18, 2022 • edited Loading

kibanamachine commented Jul 18, 2022

1Copenut commented Jul 19, 2022

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Jul 19, 2022

Choose a reason for hiding this comment

cee-chen Jul 19, 2022

Choose a reason for hiding this comment

1Copenut Jul 19, 2022

Choose a reason for hiding this comment

cee-chen commented Jul 19, 2022 • edited Loading

kibanamachine commented Jul 19, 2022

miukimiu commented Jul 20, 2022

cchaos commented Jul 25, 2022

cee-chen commented Jul 25, 2022 • edited Loading

1Copenut commented Jul 26, 2022

cchaos commented Jul 26, 2022

github-actions bot commented Oct 25, 2022

kibanamachine commented Oct 27, 2022

kibanamachine commented Oct 27, 2022

miukimiu commented Oct 28, 2022

florent-leborgne commented Oct 28, 2022

miukimiu commented Oct 28, 2022

florent-leborgne commented Oct 28, 2022

miukimiu commented Oct 28, 2022

florent-leborgne commented Oct 28, 2022

florent-leborgne commented Oct 28, 2022

cee-chen commented Oct 28, 2022 • edited Loading

1Copenut commented Oct 28, 2022

florent-leborgne commented Oct 31, 2022

cee-chen commented Oct 31, 2022

florent-leborgne commented Nov 2, 2022

cee-chen commented Nov 2, 2022

kibanamachine commented Nov 2, 2022

miukimiu left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 3, 2022

miukimiu commented Nov 3, 2022

cee-chen commented Nov 3, 2022 • edited Loading

cee-chen commented Nov 7, 2022

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 7, 2022

cee-chen commented Jul 7, 2022 •

edited

Loading

kibanamachine commented Jul 7, 2022 •

edited by cee-chen

Loading

The `iInCircle` icon

Prototype with the `iInCircle` and new icon

cee-chen commented Jul 18, 2022 •

edited

Loading

cee-chen commented Jul 18, 2022 •

edited

Loading

cee-chen commented Jul 19, 2022 •

edited

Loading

cee-chen commented Jul 25, 2022 •

edited

Loading

cee-chen commented Oct 28, 2022 •

edited

Loading

cee-chen commented Nov 3, 2022 •

edited

Loading