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

[Amsterdam] Remove borders from flyouts and popover arrows #3477

Merged
merged 20 commits into from
May 29, 2020

Conversation

johnbarrierwilson
Copy link
Member

@johnbarrierwilson johnbarrierwilson commented May 13, 2020

Summary

A. Removes the left border on flyouts to better match other Amsterdam aesthetics.

Flyout

Before After
image image

B. Also removes the borders from popover arrows while improving the aesthetic to better match the drop shadow of the related panel.

Bottom

Before After
image image

Top

Before After
image image

Left

Before After
image image

Right

Before After
image image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
    - [ ] Props have proper autodocs
    - [ ] Added documentation 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

@johnbarrierwilson
Copy link
Member Author

johnbarrierwilson commented May 13, 2020

Working on the checklist before moving out of draft status

@kibanamachine
Copy link

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

@johnbarrierwilson johnbarrierwilson marked this pull request as ready for review May 14, 2020 03:15
@kibanamachine
Copy link

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

CHANGELOG.md Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

cchaos and others added 2 commits May 14, 2020 13:46
Easier override of border with variable
Change template %eui-flyout to mixin euiFlyout
@johnbarrierwilson
Copy link
Member Author

Just making note here that clip is now depricated. Will need to rework using clip-path and possibly use @include internetExplorerOnly() to remove shadow for IE.

@kibanamachine
Copy link

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

@johnbarrierwilson
Copy link
Member Author

I added @include internetExplorerOnly{} to disable the arrow shadows. Everything else checks out in all browsers.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@johnbarrierwilson johnbarrierwilson self-assigned this May 26, 2020
@cchaos cchaos self-requested a review May 26, 2020 15:00
@johnbarrierwilson
Copy link
Member Author

@cchaos Ready for another round/review! 😁

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented May 27, 2020

Hmm, it looks like the left and right ones may need some more tweaking.

Screen Shot 2020-05-27 at 13 31 45 PM

Screen Shot 2020-05-27 at 13 31 39 PM

@johnbarrierwilson
Copy link
Member Author

That's really weird—I had that issue at one point and fixed it and now I can't reproduce it. Is your display scaled at all?

@cchaos
Copy link
Contributor

cchaos commented May 27, 2020

No, and I just used the build link above in Chrome.

@cchaos
Copy link
Contributor

cchaos commented May 27, 2020

You might just want to overcompensate?

@johnbarrierwilson
Copy link
Member Author

I increased the arrow compensation by 1 in order to overcompensate. Let me know if that fixes it for you. I was still having trouble re-producing.

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented May 28, 2020

I actually noticed played directly in the dev tools, and it looks related to the position of the arrow not the clip mask. So I adjusted all the arrows to be 1px closer to the popover panel.
Screen Recording 2020-05-28 at 01 50 PM

But like you, I didn't see it locally, but it seems to always exist in the build, so will double-check that it's fixed in the build once CI runs it.

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented May 28, 2020

^^ That seemed to work 🎉

@johnbarrierwilson Do you want to do a final pass on the shadow again?

@kibanamachine
Copy link

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

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.

👍 LGTM!

@kibanamachine
Copy link

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

@johnbarrierwilson
Copy link
Member Author

Sweet! Looks good on my end! Thanks @cchaos!

@johnbarrierwilson johnbarrierwilson merged commit e319819 into elastic:master May 29, 2020
@johnbarrierwilson johnbarrierwilson deleted the amsterdam/borders branch May 29, 2020 21:33
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