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.

Sign up for GitHub

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

Jump to bottom

[Amsterdam] Updating shadows #3428

Merged
merged 7 commits into from
May 13, 2020
Merged

[Amsterdam] Updating shadows #3428

merged 7 commits into from
May 13, 2020

Conversation

daveyholler
Copy link
Contributor

Summary

  • Updates shadow styles to improve smoothness and their appearance on a variety of backgrounds.
  • The shadows now default to using black as their base color rather than a dark-tinted blueish color.
  • Sets a warning that the opacity variable will be deprecated.
  • Opacity values for dark themes are calculated based off a base value

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

@kibanamachine
Copy link

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

@johnbarrierwilson johnbarrierwilson mentioned this pull request May 6, 2020
Closed
@cla-checker-service
Copy link

cla-checker-service bot commented May 7, 2020 •
edited
Loading

💚 CLA has been signed

@kibanamachine
Copy link

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

@daveyholler daveyholler force-pushed the am/updating-shadows branch 2 times, most recently from b406fab to 4e67af2 Compare May 7, 2020 21:17
@kibanamachine
Copy link

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

cchaos
cchaos reviewed May 7, 2020
View reviewed changes
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.

Just a quick couple things on the aesthetics, and I'll be pushing up a PR for some code refactoring

src/themes/eui-amsterdam/global_styling/mixins/_shadow.scss Outdated Show resolved Hide resolved
src/themes/eui-amsterdam/global_styling/mixins/_shadow.scss Outdated
Comment on lines 17 to 14
@mixin euiSlightShadow($color: $euiShadowColor, $opacity: 0) {
@include opacityWarning;
box-shadow:
0 .8px .8px rgba($color, shadowOpacity(.04)),
0 2.3px 2px rgba($color, shadowOpacity(.03)),
0 5.4px 5px rgba($color, shadowOpacity(.02)),
0 18px 17px rgba($color, shadowOpacity(.01));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the one that feels a little overkill to me. It's called slight but has 4 layers with the largest being 18px long. When I audit for usages of euiSlightShadow(), I get:

  1. EuiButton/EuiButtonGroup: EUI only. Amsterdam removes the shadow
  2. EuiKeyPadMenuItem: Component itself hasn't been altered for Amsterdam yet and is still using the border style panel
  3. EuiStepHorizontal: With the color altered to blue, it's hardly visible at all and could probably just be removed
  4. form/euiCustomControl: These are all the form elements like checkboxes, radios, and radio style handles for sliders. These are all roughly 16x16 components, pretty small, so it's odd to have such a large shadow.

I'd suggest keeping the slight shadow very slight. It's possible once all those above components have been touched we may have no need for it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I'll clean that up.

src/themes/eui-amsterdam/global_styling/mixins/_shadow.scss Outdated Show resolved Hide resolved
src/themes/eui-amsterdam/global_styling/functions/_shadows.scss
Comment on lines +4 to +10
@function shadowOpacity($opacity) {
@if (lightness($euiTextColor) < 50) {
@return $opacity * 1;
} @else {
@return $opacity * 2.5;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@cchaos cchaos force-pushed the am/updating-shadows branch from 4e67af2 to 9869c6d Compare May 7, 2020 22:18
@kibanamachine
Copy link

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

daveyholler and others added 6 commits May 12, 2020 10:09
@daveyholler
Working on shadows.
3e354d8
@daveyholler
Fixing a missing px unit on small shadows.
e6701ac
@daveyholler
Fixing a unitless value on large shadows.
cf142d8
@daveyholler
Code refactor
71e8d47 
- Removed modal override and changed the default theme usage since it wasn’t really changing the opacity noticeabley anyway
- Moved warning messages to each mixin … sigh
- Made the docs layout look more Amsterdamy
@daveyholler
Removing the largest value from large shadows.
a86497f
@daveyholler
Adding missing semicolons
bbf5d15
@daveyholler daveyholler force-pushed the am/updating-shadows branch from 04bd4dc to bbf5d15 Compare May 12, 2020 17:27
@daveyholler daveyholler marked this pull request as ready for review May 12, 2020 17:28
@daveyholler daveyholler requested a review from cchaos May 12, 2020 17:28
@kibanamachine
Copy link

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

cchaos
cchaos approved these changes May 12, 2020
View reviewed changes
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! Shadows are really nice on things like cards and such.

src/themes/eui-amsterdam/global_styling/mixins/_shadow.scss Outdated Show resolved Hide resolved
@daveyholler
Copy link
Contributor Author

Jenkins test this

@kibanamachine
Copy link

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

@daveyholler daveyholler merged commit ff4447e into elastic:master May 13, 2020
@thompsongl thompsongl mentioned this pull request Jun 3, 2020
Merged
@clintandrewhall clintandrewhall mentioned this pull request Apr 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daveyholler @kibanamachine @cchaos