Fixed description prop in EuiTable#4754
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
79a2360 to
b95ee47
Compare
elizabetdev
left a comment
There was a problem hiding this comment.
Hi @hetanthakkar1,
Thanks for opening this PR. I tested and it works. 🎉
From a design perspective, we need a visual hint to show that the column headers contain a description. So we can take this example as a design solution: https://elastic.github.io/eui/#/tabular-content/tables#adding-sorting-to-a-table
As the above example shows, when a column header has a description we should include the following icon:
<EuiIcon
size="s"
color="subdued"
type="questionInCircle"
/>Then we should update all the examples that are using a tooltip like this one, to use a description prop rather than passing a tooltip.
So instead of adding a tooltip:
eui/src-docs/src/views/tables/sorting/sorting.js
Lines 95 to 115 in dbcf763
We should:
{
field: 'github',
name: 'Github',
description: 'Their mascot is the Octokitty',
render: (username) => (
<EuiLink href={`https://github.com/${username}`} target="_blank">
{username}
</EuiLink>
),
},|
@miukimiu What do you think |
Going back to the original issue:
I read "presented as a title over the column header" to mean that we will use the native browser It makes more sense to me to simply do |
|
@thompsongl You are correct! WIll make the required changes |
b95ee47 to
75a94ae
Compare
|
@thompsongl Please review |
There was a problem hiding this comment.
LGTM! 🎉
Tested locally in Chrome, Safari, Edge, and Firefox.
As @thompsongl mentioned
I read "presented as a title over the column header" to mean that we will use the native browser title element hover effect, not an EuiToolTip. The action item button case is different in that it's not in the header, and it does not describe a column (it describes the action list).
It makes sense to use the native browser title and it also makes sense to leave the "Adding sorting to a table" example with the tooltips. So in case, consumers need a more detailed description they can use that pattern. 🙂
Let's wait for @thompsongl approval and we're good to merge.
|
@miukimiu Can you please take a look at this too: #4753 (comment) |
|
@thompsongl @myasonik Can you please review!? I think I have made the necessary changes but feel free to correct me |
myasonik
left a comment
There was a problem hiding this comment.
@hetanthakkar1 Thanks for the quick turnaround! It's on the right track!
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4754/ |
|
@thompsongl can you please review |
thompsongl
left a comment
There was a problem hiding this comment.
LGTM; Thanks for sticking with all the change requests!
Let's have @myasonik take another look before merging
Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>

Summary
Fixed issue #4333 : Description prop in EuiTable for column description and updated test snapshots.
For a brief overview of implementation details I have implemented this feature similar to
DefaultItemActionas mentioned in this comment: #4333 (comment)Checklist
Added documentationChecked for breaking changes and labeled appropriatelyChecked for accessibility including keyboard-only and screenreader modes