Skip to content

Fixed description prop in EuiTable#4754

Merged
myasonik merged 5 commits intoelastic:masterfrom
hetanthakkar:basicTable
Apr 29, 2021
Merged

Fixed description prop in EuiTable#4754
myasonik merged 5 commits intoelastic:masterfrom
hetanthakkar:basicTable

Conversation

@hetanthakkar
Copy link
Copy Markdown
Contributor

@hetanthakkar hetanthakkar commented Apr 25, 2021

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 DefaultItemAction as mentioned in this comment: #4333 (comment)

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

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?

Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

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

Screenshot 2021-04-26 at 13 08 53

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:

{
field: 'github',
name: (
<EuiToolTip content="Their mascot is the Octokitty">
<span>
Github{' '}
<EuiIcon
size="s"
color="subdued"
type="questionInCircle"
className="eui-alignTop"
/>
</span>
</EuiToolTip>
),
render: (username) => (
<EuiLink href={`https://github.com/${username}`} target="_blank">
{username}
</EuiLink>
),
},
.

We should:

{
  field: 'github',
  name: 'Github',
  description: 'Their mascot is the Octokitty',
  render: (username) => (
    <EuiLink href={`https://github.com/${username}`} target="_blank">
    {username}
    </EuiLink>
  ),
},

@hetanthakkar
Copy link
Copy Markdown
Contributor Author

@miukimiu What do you think

@thompsongl
Copy link
Copy Markdown
Contributor

That result in display of both the title and description like this

Going back to the original issue:

A description of the column (will be presented as a title over the column header)

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 more sense to me to simply do title={description || innerText} in the locations in question, and remove the new EuiToolTip elements.

@hetanthakkar
Copy link
Copy Markdown
Contributor Author

@thompsongl You are correct! WIll make the required changes

@hetanthakkar
Copy link
Copy Markdown
Contributor Author

@thompsongl Please review

Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

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.

@hetanthakkar
Copy link
Copy Markdown
Contributor Author

@miukimiu Can you please take a look at this too: #4753 (comment)

@hetanthakkar
Copy link
Copy Markdown
Contributor Author

@thompsongl @myasonik Can you please review!? I think I have made the necessary changes but feel free to correct me

Copy link
Copy Markdown
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

@hetanthakkar1 Thanks for the quick turnaround! It's on the right track!

@myasonik
Copy link
Copy Markdown
Contributor

jenkins test this

@myasonik myasonik requested review from myasonik and thompsongl April 28, 2021 20:06
@kibanamachine
Copy link
Copy Markdown

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

@hetanthakkar
Copy link
Copy Markdown
Contributor Author

@thompsongl can you please review

Copy link
Copy Markdown
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM; Thanks for sticking with all the change requests!
Let's have @myasonik take another look before merging

@myasonik myasonik merged commit 0a3badd into elastic:master Apr 29, 2021
cchaos pushed a commit that referenced this pull request May 4, 2021
Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co>
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.

5 participants