-
Notifications
You must be signed in to change notification settings - Fork 861
[Visual Refresh] New shadow tokens and utilities #8962
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
[Visual Refresh] New shadow tokens and utilities #8962
Conversation
- these shadows are not in use yet, but there are plans to use them in the future for hovered panels/cards
- separates SCSS shadows per theme
a1aa020 to
46612bd
Compare
|
Thanks for your patience and all the huge work @mgadewoll! A couple of comments / questions:
Other than that, everything else looks fine for me. |
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.
Approving not to block, things that were mentioned don't look as a blocker to me.
@ek-so Thanks for the review!
|
|
ℹ️ Marking this PR as "Draft" again due to an additional concern we haven't considered yet: Potential dimension or position calculations that might be impacted by the floating border difference 🫠 |
After looking into the raised scenario on flyouts, it showed that the use case was specific to flyout management. The issue happened due to a border being added in light mode on the parent flyout while the child flyout is being opened which affected the opening animation origin and caused a jump. Adding the border on the child flyout instead prevents the issue. As this was a very localized issue that can easily be prevented, we're good to proceed with the review of this implementation proposal. |
acstll
left a comment
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.
Thank you @mgadewoll for going the extra mile with this one. I think it's working great!
I followed the QA process (except for testing in Kibana directly) and it's all working as expected. I also checked the values match the spec, and that values in JSON and SCSS are correct and aligned (used an LLM for both of these).
Most comments are regarding the docs, and one regarding an API. Everything else looks good…
Thanks again!
packages/website/docs/getting-started/theming/tokens/shadows/shadows_table.tsx
Outdated
Show resolved
Hide resolved
packages/website/docs/getting-started/theming/tokens/shadows/index.mdx
Outdated
Show resolved
Hide resolved
packages/website/docs/getting-started/theming/utilities/shadows/index.mdx
Show resolved
Hide resolved
packages/website/docs/getting-started/theming/utilities/shadows/index.mdx
Outdated
Show resolved
Hide resolved
packages/website/docs/getting-started/theming/utilities/shadows/index.mdx
Show resolved
Hide resolved
packages/website/docs/getting-started/theming/utilities/shadows/index.mdx
Show resolved
Hide resolved
acstll
left a comment
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 for the updates! 💅
I spotted 2 console.logs, not approving yet in case you didn't leave them there intentionally.
packages/website/docs/getting-started/theming/utilities/shadows/index.mdx
Outdated
Show resolved
Hide resolved
packages/website/docs/getting-started/theming/tokens/shadows/shadows_table.tsx
Outdated
Show resolved
Hide resolved
💚 Build SucceededHistory
cc @mgadewoll |
💚 Build Succeeded
History
cc @mgadewoll |
acstll
left a comment
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 again for the updates, amazing work ✨
Summary
Implements https://github.com/elastic/eui-private/issues/195
Important
This PR merged into a feature branch.
Note
This PR is continuing the work started in #8880 but implements a different approach for applying floating borders.
Selected implementation approach
The previous PR implemented a different approach to handling shadow tokens, which aligned directly with design specs and included an additional layer for Dark mode shadows for the expected floating border.
This new PR now implements an alternative approach: the floating border is not part of the shadow but instead added next to the
box-shadowstyle as standaloneborderstyle.Both approaches have their own benefits and downsides, there is no perfect solution. The ultimate decision to implement floating borders as standalone border was based on a comparison of possible impact and maintenance.
The deciding factor for the
borderapproach was that it provides flexibility for the cases that need it while thebox-shadowapproach is more rigid which could lead to blocking issues if they arise.box-shadowonly- no dimension changes between color modes
- shadow placement outside of the border-box prevents duplicate borders in compositions "by accident"
borderon customizations which will result in double borders)-
borderbecausebox-shadowsits outside the border-box-
box-shadowfloating border can be cut off if an ancestor hasoverflow: hidden-
border- easy to adjust in customizations
- supports side-specific borders or removing floating borders entirely
-
-
ℹ️ For the in depth comparison of the approaches with examples see this comment.
Changes
Note
The new tokens have been added as SCSS and JSON exports as well.
⚠️ The new shadow utils have not been added to SCSS, only existing utils have been updated. This is because EUI does not officially supporting SCSS anymore.
updates existing shadow utils to use new shadow tokens
euiShadoweuiShadowXSmalleuiShadowSmalleuiShadowMediumeuiShadowLargeeuiShadowXLargeeuiShadowFlateuiSlightShadowHoveradds new shadow hover util
euiShadowHoverseparated shadow SCSS per theme instead of shared through
eui-theme-commonadds documentation for shadow tokens and utils
Why are we making this change?
Design feature: Updating shadows as part of the Visual Refresh project.
Screenshots
HCM
Impact to users
🟢 The are no updates needed on consumer side.
eui-theme-commonpackage. (this generally is not an issue as it's a direct dependency oneuiit might only be an issue with different versions ofeuiavailable)QA
ℹ️ The changes have also been deployed to Kibana here (login)
General checklist
Checked in mobileChecked for accessibility including keyboard-only and screenreader modesProps have proper autodocs (using@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesAdded or updated jest and cypress testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)