-
Notifications
You must be signed in to change notification settings - Fork 829
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
[EuiTour] Convert to Emotion styling #6087
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
[data-popover-arrow='top'] { | ||
&:before { | ||
${logicalCSS( | ||
'border-top-color', | ||
backgroundColor(euiTheme.colors.lightestShade, colorMode) | ||
)}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[not a blocking comment, just noting something] I know this is just a translation from Sass so this is already happening in prod, but it looks like left/right arrows that are close to the colored footer also have incorrect arrow colors:
tbh not a huge deal and i'm not sure if there's a way of programmatically detecting this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember not being able to account for this when writing the Sass styles originally, but maybe we can do some calculations now? I'll look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to timebox / no worries if it's too much of a hassle, we can punt!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember not being able to account for this when writing the Sass styles originally, but maybe we can do some calculations now? I'll look.
It would be great to support this. 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't quite got this yet but don't want to hold up this PR any longer. I'll do a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great to me, I have a few nits and questions, but otherwise QA looks great, logical properties all look good, and I'm really digging how the Emotion styles were organized!
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @thompsongl!
Tested in Chrome, Safari, Edge, and Firefox. LGTM! 🎉
[data-popover-arrow='top'] { | ||
&:before { | ||
${logicalCSS( | ||
'border-top-color', | ||
backgroundColor(euiTheme.colors.lightestShade, colorMode) | ||
)}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember not being able to account for this when writing the Sass styles originally, but maybe we can do some calculations now? I'll look.
It would be great to support this. 👍🏽
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One very minor comment left - since it's small, I'll go ahead and approve. Other changes look great, thanks!
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6087/ |
Summary
Like it says on the tin.
Also,
onPositionChange
callback toEuiPopover
for getting arrow placementuseEuiTour
because it incorrectly expectedonFinish
to be part of each step configChecklist