components: Remove @emotion/css from Elevation
#33098
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
sarayourfriend
added
[Feature] Component System
|
Size Change: +674 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
sarayourfriend
force-pushed
the
remove/emotion-css-elevation
branch
from
9bbed51 to
d952243
Compare
ciampo
reviewed
Jul 2, 2021
|
|
||
| export const ElevationView = styled( 'div', { | ||
| shouldForwardProp: ( propName ) => | ||
| ! DO_NOT_FORWARD.includes( propName as string ), |
There was a problem hiding this comment.
Should we also add the isPropValid() check, following the example in the official docs?
Comment on lines
+18
to
+26
| const DEFAULT_PROPS: ElevationViewProps = { | ||
| isInteractive: false, | ||
| offset: 0, | ||
| value: 0, | ||
| active: null, | ||
| focus: null, | ||
| hover: null, | ||
| borderRadius: 'inherit', | ||
| }; |
There was a problem hiding this comment.
Note to self:
This is the approach that was agreed in order to set the default values for props only once per component (these default values would otherwise be set also in when computing the styles) in a clear and explicit way.
The styles, in fact, assume that all props are always defined (and does so through the Required TypeScript utility)
| will-change: box-shadow; | ||
| opacity: ${ CONFIG.elevationIntensity }; | ||
| ${ renderTransition } | ||
| ${ reduceMotion( 'transition' ) } |
There was a problem hiding this comment.
Was this added as part of this PR, or did the Elevation component already handle reduced-motion ?
There was a problem hiding this comment.
It was added as part of this, the Elevation component incorrectly did not use this before.
| @@ -8,19 +8,19 @@ export type Props = { | |||
| /** | |||
| * Renders the active (interaction) shadow value. | |||
| */ | |||
| active?: number; | |||
| active?: number | null; | |||
There was a problem hiding this comment.
Should we add @default tags matching the DEFAULT_PROPS defined in packages/components/src/elevation/component.tsx ?
ciampo
requested changes
Jul 2, 2021
There was a problem hiding this comment.
I actually just realised that something seems wrong with the component's behaviour when focused — focus styles don't seem to be applied correctly.
This should be reproducible when comparing the Elevation's Focus storybook story from this branch against the one in production.
Could it be related to the shouldForwardProps option?
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Part of #30503 (comment)
How has this been tested?
Storybook, everything all the props etc should all work as they worked before.
Types of changes
Non breaking change
Checklist:
*.native.jsfiles for terms that need renaming or removal).