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

Adds display prop to EuiTooltip for common display block needs #4148

Merged
merged 11 commits into from
Oct 21, 2020

Conversation

snide
Copy link
Contributor

@snide snide commented Oct 15, 2020

Summary

Fixes #4086 . Adds a display prop to EuiToolTip for the common need of making the anchors block level elements.

image

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

@snide
Copy link
Contributor Author

snide commented Oct 15, 2020

Made this in a pair session with @glitteringkatie! She's gonna finish it up and add tests for me!

@kibanamachine
Copy link

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

@cla-checker-service
Copy link

cla-checker-service bot commented Oct 19, 2020

💚 CLA has been signed

@glitteringkatie
Copy link
Contributor

glitteringkatie commented Oct 19, 2020

Added tests! 🙌 It doesn't seem like I did the right things when pushing as it shows 2 different Katie Hugheses wrote and committed and neither have my picture and I've already signed a contributor agreement 🤔 I might need some help making sure I have the right workflow for dealing with branches forks

@andreadelrio
Copy link
Contributor

Added tests! 🙌 It doesn't seem like I did the right things when pushing as it shows 2 different Katie Hugheses wrote and committed and neither have my picture and I've already signed a contributor agreement 🤔 I might need some help making sure I have the right workflow for dealing with branches forks

@snide don't forget to add Katie to
https://github.com/orgs/elastic/teams/eui-design

@glitteringkatie
Copy link
Contributor

snide don't forget to add Katie to
https://github.com/orgs/elastic/teams/eui-design

hehe thanks @andreadelrio

@kibanamachine
Copy link

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

@snide
Copy link
Contributor Author

snide commented Oct 19, 2020

@glitteringkatie is added to that team, but I think the CLA portion is handled differently. My guess is that we'll need @chandlerprall's help there.

@snide snide marked this pull request as ready for review October 19, 2020 22:11
@chandlerprall
Copy link
Contributor

Looks like that commit (8f15f3a) wasn't made with the email address associated with the @glitteringkatie github account:

Katie Hughes authored and Katie Hughes committed 18 hours ago

commit 8f15f3a (snide/tooltip_display)
Author: Katie Hughes [email protected]
Date: Mon Oct 19 14:14:30 2020 -0700

Add test for display=block

Email address needs to be set globally via git config --global user.email "[email protected]"

@glitteringkatie
Copy link
Contributor

Email address needs to be set globally via git config --global user.email "[email protected]"

I see what went wrong--I did git config --global --edit and updated it but didn't uncomment the lines 🤦‍♀️

@kibanamachine
Copy link

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

@glitteringkatie
Copy link
Contributor

I resolved the changelog conflicts this morning and there's already another one! Is it better practice to leave fixing the changelog conflicts until the PR is approved/close to being approved assuming all it is is one added line?

@cchaos
Copy link
Contributor

cchaos commented Oct 20, 2020

I resolved the changelog conflicts this morning and there's already another one! Is it better practice to leave fixing the changelog conflicts until the PR is approved/close to being approved assuming all it is is one added line?

It's up to you really. I think your read on it is pretty well aligned. Personally, like those red notification dots, hate seeing conflicts so I'm constantly hitting that "Resolve conflicts" button. Also, it's a quick way to get the branch rebased with master.

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.

👋 Welcome Katie! 😆

Looks like we've also got dueling PR's going. First one to get an approval wins a lollipop 🍭

.gitignore Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src-docs/src/views/tool_tip/tool_tip.js Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.test.tsx Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
@snide snide requested a review from cchaos October 20, 2020 23:32
@kibanamachine
Copy link

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

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! LGTM. Just saw a couple extra minor things.

src-docs/src/views/tool_tip/tool_tip.js Outdated Show resolved Hide resolved
src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@glitteringkatie glitteringkatie merged commit 72214af into elastic:master Oct 21, 2020
@cchaos cchaos mentioned this pull request Oct 21, 2020
10 tasks
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.

[EuiTooltip] Add display prop like EuiPopover
6 participants