fix: Elevation context wrapping #14593
Conversation
peter-sanderson
requested review from
jvaclavik and
seibei-iguchi
as code owners
| ); | ||
| }; | ||
|
|
||
| const NewModalBase = (props: NewModalProps) => ( | ||
| <ElevationContext baseElevation={prevElevation[MODAL_ELEVATION]}> |
There was a problem hiding this comment.
Here the prevElevation must be used, as ElevationContext sets the level of the plane, on which NewModal sits on.
If we want NewModal to be 0 it must sit on plane of elevation -1
peter-sanderson
force-pushed
the
fix_elevation_for_new_modal
branch
from
f133c61 to
aad54be
Compare
| )} | ||
| </Header> | ||
| <Container | ||
| $elevation={elevation} |
There was a problem hiding this comment.
MODAL_ELEVATION no longer needs to be hard-coded as we can use value from useElevation now, because we are in nested component and the Context (ElevationContext) of this component is correctly set by React.
| @@ -131,7 +131,7 @@ type NewModalProps = AllowedFrameProps & { | |||
| 'data-testid'?: string; | |||
| }; | |||
|
|
|||
| const NewModalBase = ({ | |||
| const _NewModalBase = ({ | |||
There was a problem hiding this comment.
I really don't like this naming:D There must be something better
There was a problem hiding this comment.
Feel free to come up with better name / rename it later ;)
| {children} | ||
| </Body> | ||
| </ScrollContainer> | ||
| <ShadowBottom backgroundColor={modalBackgroundColor} /> |
There was a problem hiding this comment.
Problem here is, that ShadowBottom component is placed in JSX in the context of NewModalBase parent component.
The fact its wrapped in <ElevationContext ... does not set its context to it, but keeps the context of the NewModalBase.
We have to create new component function, so the new context is created and the correct elevation value is retrieved from it (in this case the value -1 + 1 = 0 <-- correct.
The reason why it "worked" (kinda), is because useElevation has a default behavior and fallbacks to 0 (which resulted in this case in 0 + 1 = 1 <-- incorrect.
In the light of this issue, I kinda think we should remove default behavior and throw error if useElevation is used without context being set.
Defaults are evil.
cc @jvaclavik ☝️
|
QA OK Info:
|
Fixes the incorrect use of React Context so the elevation value is passed down correctly.
This also fixes the base elevation of modal, as elevation context sets the elevation of the plane the
NewModalis onto.