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

[EuiBasicTable] add nameTooltip API #7963

Open
alexwizp opened this issue Aug 14, 2024 · 3 comments
Open

[EuiBasicTable] add nameTooltip API #7963

alexwizp opened this issue Aug 14, 2024 · 3 comments
Assignees
Labels
accessibility feature request low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed

Comments

@alexwizp
Copy link
Contributor

This feature request is based on this comment and suggests adding a new nameTooltip API for EuiBasicTable.

Example:

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>
    ),
  },
];
@acstll
Copy link
Contributor

acstll commented Jan 14, 2025

I started implementing this in #8273 and I have a question regarding the actual API. (I went and read thru all previous discussion I could find).

I understand the proposed API is flexible enough while keeping things under control. And I believe it's also decoupled from the implementation. If we use EuiIconTip, the props are mapped like this:

nameTooltip: {
    content: 'Their mascot is Octokitty', // passed to EuiIconTip's `content` [same]
    icon: 'questionInCircle', // passed to EuiIconTip's `type`
    iconProps: { color: 'subdued' }, // passed to EuiIconTip's `iconProps` [same]
    tooltipProps: { position: 'bottom' }, // spreaad to EuiIconTip's subset of the EuiToolTip props
},

I think this is good. But I'm also thinking whether things would be simpler if the shape of nameTooltip would match exactly the props from EuiIconTip.

The implementation would be simpler, there would be no need for a new shape/type, but it would be somehow bound to EuiIconTip.

Is this a silly idea? What do you think?

@cee-chen
Copy link
Contributor

cee-chen commented Jan 22, 2025

Definitely not a silly idea, but (unfortunately) it's a bit of a question of consistency rather than optimal naming at this point. tooltipProps is a fairly commonly used API across multiple components in EUI at this point, so it's one that's more familiar for devs to reach for (as opposed to having an entire object be the props for a dogfooded component, which isn't a pattern we've used previously in EUI).

Additionally, my 2c is I think a custom object structure is more flexible in terms of typing than doing something like nameTooltip?: EuiIconTipProps;. It allows us to set default props for props that may otherwise be required by the underlying component. That's not necessarily a dealbreaker, just a typing hassle, so I think the consistency argument above is a slightly stronger one.

@acstll
Copy link
Contributor

acstll commented Jan 23, 2025

it's a bit of a question of consistency rather than optimal naming at this point

this is the answer I was looking for — there's really no point in introducing a new API just for the sake of a simpler implementation, I will refactor back…

@cee-chen thanks for chiming in 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility feature request low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed
Projects
None yet
Development

No branches or pull requests

4 participants