eui
eui copied to clipboard
[docs][a11y] `EuiBasicTable` examples updated with tooltip usage
Our current EuiBasicTable examples need to be updated to be more accessible using EuiIconTip.
Preview staging links for this PR:
- Docs site: https://eui.elastic.co/pr_7861/
- Storybook: https://eui.elastic.co/pr_7861/storybook
@alexwizp doesn't this reduce the trigger area of the tooltip content? 🫤 This feels like a usability net loss for mouse users, just IMO
@cee-chen, do we need to include any changelog for the documentation updates?
@cee-chen / @1Copenut
doesn't this reduce the trigger area of the tooltip content? 🫤 This feels like a usability net loss for mouse users, just IMO
I'm a bit confused, as we already use this pattern across cloud and kibana. I think it's ok if the trigger only happens upon icon focus. This aligns with how we use EuiIconTip in other places.
Ahh interesting 😬 If we already use that pattern in Kibana, and it's the better a11y experience, I guess we might as well update EUI's docs to match - but I was under the impression that the EUI example is the better one in terms of UX?
@cee-chen I think the main issue with the current example is that it doesn't allow the tooltip to open when navigating with the keyboard. This is a significant defect. On the other hand the functionality to open it with mouse is still retained when hovering over the icon. I believe this is correct.
From my point of view If you expected some action when hovering over the text, text should look visually different e.g.
@1Copenut please help us here
This is a tough one, because it's both. We must have a tootlip that takes keyboard focus so it's available for keyboard users, but also need a target space at least 24px by 24px that activates the tooltip on hover. Specifically I'm looking at:
Separating the tooltip into its own button solves the keyboard issue, but the SVG is going to have to be larger to meet the required surface area for mouse hover. I'm not going to be much help defining how that looks, so I'll phone a friend and ask @MichaelMarcialis for his input.
I think my personal vote would be a new API called nameTooltipContent and nameTooltipProps that tells EuiBasicTable to conditionally wrap the column header in a tooltip. Example usage:
const columns = [
{
field: 'github',
name: (
<>
Github{' '}
<EuiIcon
size="s"
color="subdued"
type="questionInCircle"
className="eui-alignTop"
/>
</>
),
nameTooltipContent: 'Their mascot is Octokitty',
nameTooltipProps: {
position: 'bottom',
},
render: (username: User['github']) => (
<EuiLink href="#" target="_blank">
{username}
</EuiLink>
),
},
];
For other components where we have similar tooltipContent and tooltipProps APIs, see #7690
@cee-chen from my point of view the suggested API may not so ideal. The information icon is still obscured when the text is truncated. If we opt to revise the API, I think we should place the icon as a sibling to the sort icon, as suggested by @1Copenut in the created issues.
maybe:
const columns = [
{
field: 'github',
name: 'Github',
"append | actions": [
<EuiIconTip
size="s"
color="subdued"
type="questionInCircle"
className="eui-alignTop"
/>
],
render: (username: User['github']) => (
<EuiLink href="#" target="_blank">
{username}
</EuiLink>
),
},
];
Great point about the icon and truncation! How about nameTooltipContent, nameTooltipIcon, and nameTooltipProps?
const columns = [
{
field: 'github',
name: 'Github',
nameTooltipIcon: 'questionInCircle',
nameTooltipContent: 'Their mascot is Octokitty',
nameTooltipProps: { position: 'bottom' },
render: (username: User['github']) => (
<EuiLink href="#" target="_blank">
{username}
</EuiLink>
),
},
];
🤦 Oh or actually a single object of multiple optional configurations would be even better:
const columns = [
{
field: 'github',
name: 'Github',
nameTooltip: {
content: 'Their mascot is Octokitty',
icon: 'questionInCircle', // default icon, but can be customized
iconProps: { color: 'subdued' }, // allows customizing the icon color/size etc if needed
tooltipProps: { position: 'bottom' }, // allows additional tooltip configuration
},
render: (username: User['github']) => (
<EuiLink href="#" target="_blank">
{username}
</EuiLink>
),
},
];
Also thought about nameTooltip, but then decided to propose a more generic API. Maybe someone needs to add some extra buttons here, or more than one tooltip, such as an info tooltip and a warning tooltip.
I would strongly rather not bloat the scope and open up an infinite possibility of actions. I'm concerned about that long term because we don't want consumers doing super crazy things in table headers, which IMO should be fairly limited as it comes with accessibility concerns (as we've seen here).
I also strongly prefer sticking with the tooltip API as we have similar APIs all over EUI, so it's an established pattern at this point.
@alexwizp if no objections I'd be happy to take over this PR or open another with the new API for table header tooltips!
@cee-chen sure! thanks
Separating the tooltip into its own button solves the keyboard issue, but the SVG is going to have to be larger to meet the required surface area for mouse hover. I'm not going to be much help defining how that looks, so I'll phone a friend and ask @MichaelMarcialis for his input.
@1Copenut: Good question. While having a 24px tall tooltip anchor would be beneficial for mouse-based users, we need to weigh that against how adjacent content will be affected. Seeing as the line height for the basic table headings is only 16px, throwing a 24px tall element in the mix would unintentionally increase header row height (without additional CSS countermeasures) and could potentially look odd if the icon is sized to match (i.e. might look too large visually in comparison to the adjacent text).
What about the following exception from the W3C sizing rule?
Inline: The target is in a sentence or its size is otherwise constrained by the line-height of non-target text;
Does this instance qualify for such an exception? I think it does, but I'm not sure if my temptation to avoid opening a can of worms is biasing me 😉
@MichaelMarcialis The inline exception is interesting. If we kept the 16px height and possibly adding a bit more horizontal padding to increase the hit space, that seems like a good compromise. I agree that having a 24px tall icon in a 16px line-height is going to be visually disruptive.
@alexwizp if no objections I'd be happy to take over this PR or open another with the new API for table header tooltips!
@cee-chen I can't find it, did we created a new issue for that finally?
@alexwizp I didn't end up making a new issue/PR apologies, although it's in my mental backlog. Feel free to create an issue if you'd like something more tangible :)
I'm currently heads down on completing the long-standing Emotion migration for the next 2 weeks, but I'd like to grab this after that!