Skip to content

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

Merged
glitteringkatie merged 11 commits intoelastic:masterfrom
snide:tooltip_display
Oct 21, 2020
Merged

Adds display prop to EuiTooltip for common display block needs#4148
glitteringkatie merged 11 commits intoelastic:masterfrom
snide:tooltip_display

Conversation

@snide
Copy link
Copy Markdown
Contributor

@snide snide commented Oct 15, 2020 •
edited
Loading

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
Copy Markdown
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
Copy Markdown

kibanamachine commented Oct 15, 2020

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

@cla-checker-service
Copy link
Copy Markdown

cla-checker-service bot commented Oct 19, 2020 •
edited
Loading

💚 CLA has been signed

@glitteringkatie
Copy link
Copy Markdown
Contributor

glitteringkatie commented Oct 19, 2020 •
edited
Loading

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
Copy Markdown
Contributor

andreadelrio 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

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

@glitteringkatie
Copy link
Copy Markdown
Contributor

glitteringkatie commented Oct 19, 2020

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

hehe thanks @andreadelrio

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 19, 2020

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

@snide
Copy link
Copy Markdown
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
Copy Markdown
Contributor

chandlerprall commented Oct 20, 2020

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 katiehughes@Katies-MacBook-Pro.local
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 "MY_NAME@example.com"

@glitteringkatie
Copy link
Copy Markdown
Contributor

glitteringkatie commented Oct 20, 2020

Email address needs to be set globally via git config --global user.email "MY_NAME@example.com"

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 20, 2020

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

@glitteringkatie
Copy link
Copy Markdown
Contributor

glitteringkatie 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?

@cchaos
Copy link
Copy Markdown
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.

cchaos
cchaos suggested changes Oct 20, 2020
View reviewed changes
Copy link
Copy Markdown
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 🍭

snide and others added 5 commits October 20, 2020 16:14
@snide @cchaos
Update CHANGELOG.md
f3ce1df 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@snide @cchaos
Update src/components/tool_tip/tool_tip.test.tsx
02faca3 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@snide @cchaos
Update src/components/tool_tip/tool_tip.tsx
ebd8ea7 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@snide
Merge remote-tracking branch 'upstream/master' into tooltip_display
11ad5ef
@snide
feedback
5cd1e85
@snide snide requested a review from cchaos October 20, 2020 23:32
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 20, 2020

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

cchaos
cchaos approved these changes Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
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.

snide and others added 2 commits October 21, 2020 11:31
@snide @cchaos
Update src-docs/src/views/tool_tip/tool_tip.js
5a8dd61 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@snide @cchaos
Update src/components/tool_tip/tool_tip.tsx
e8825a6 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Oct 21, 2020

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
Closed
10 tasks
@thompsongl thompsongl mentioned this pull request Oct 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiTooltip] Add display prop like EuiPopover

6 participants

@snide @kibanamachine @glitteringkatie @andreadelrio @chandlerprall @cchaos