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

[EuiToolTip] Add display prop #4096

Closed

Conversation

Phizzard
Copy link
Contributor

@Phizzard Phizzard commented Oct 1, 2020

Summary

This feature adds a display prop to the EuiTooltip component to be a means of quick customization for handling display values, instead of manually adding a class into the prop anchorClassName.

A similar implementation of this exists in the EuiPopover component.

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

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?

@cla-checker-service
Copy link

cla-checker-service bot commented Oct 1, 2020

💚 CLA has been signed

@cchaos
Copy link
Contributor

cchaos commented Oct 1, 2020

Welcome @Phizzard 👋
Would you please sign the CLA linked below. Be sure to use the same email address linked to your Github account.
I'll get our CI (Jenkins) started for you which will run our tests and get a preview built.
Jenkins, test this

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Also don't forget to check off or cross out items in the summary checklist. One additional thing you'll need to add is a changelog entry which you can find a link to in the checklist. Just be sure to follow the pattern in that file.

src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@cchaos
Copy link
Contributor

cchaos commented Oct 1, 2020

Oh and we'll need an additional Jest test for this new prop which you can add in the associated tool_tip.test.tsx file

@cchaos cchaos changed the title Feat/4086 tooltip display prop [EuiToolTip] Add display prop Oct 1, 2020
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Oct 13, 2020

Jenkins, test this

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Unfortunately, this isn't actually working because of the way the component renders.

src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
src/components/tool_tip/_tool_tip.scss Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Oct 19, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks @Phizzard for making those changes. One last thing is that it'd be great to see an example of this in the docs. Can you add one where it's using a <EuiButton fullWidth> component as the children?

block: 'euiToolTipAnchor--displayBlock',
};

export const DISPLAY = Object.keys(displayToClassNameMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used anywhere, safe to remove.

Suggested change
export const DISPLAY = Object.keys(displayToClassNameMap);

Comment on lines 4 to +5

- Added `display` prop to `EuiTooltip` ([#4096](https://github.com/elastic/eui/pull/4096))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Added `display` prop to `EuiTooltip` ([#4096](https://github.com/elastic/eui/pull/4096))
- Added `display` prop to `EuiTooltip` ([#4096](https://github.com/elastic/eui/pull/4096))

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it looks like you'll need to merge master to get the updated changelog file and move this to the newest master section.

@cchaos
Copy link
Contributor

cchaos commented Oct 21, 2020

Hey @Phizzard. Looks like we had dueling PR's for this one and #4148 was able to make it in first. We appreciate all the work you did on this one. Please feel free to grab another from our list and be sure the reference the issue in your PR so people can see you've got it started already.

@cchaos cchaos closed this Oct 21, 2020
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.

3 participants