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

fix: Elevation context wrapping #14593

Merged
merged 1 commit into from
Sep 27, 2024
Merged

fix: Elevation context wrapping #14593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/src/components/NewModal/NewModal.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export const NewModal: StoryObj<NewModalProps> = {
heading: 'Modal heading',
description: 'Modal description',
children:
'Quos delectus veritatis est doloribus dolor. Odit fugit omnis magni ipsam quia rem aut. Et alias sint non. Consequuntur dignissimos veritatis debitis corporis esse. Quaerat voluptatem unde aut. Iusto laborum omnis quis amet atque. Sint culpa delectus non soluta temporibus saepe. Sequi saepe corrupti aliquam ut sit assumenda aspernatur consequuntur. Ut est ullam iusto facilis voluptatibus. Sit est cum quos. Quasi deleniti non fugit iste alias consequuntur. Ullam ad ut culpa est reiciendis molestiae. Reiciendis ab veritatis a totam inventore nihil voluptatem occaecati. Quisquam atque odit quia nam. Laboriosam rem et ut. Maxime qui voluptatem voluptatem.',
'Quos delectus veritatis est doloribus dolor. Odit fugit omnis magni ipsam quia rem aut. Et alias sint non. Consequuntur dignissimos veritatis debitis corporis esse. Quaerat voluptatem unde aut. Iusto laborum omnis quis amet atque. Sint culpa delectus non soluta temporibus saepe. Sequi saepe corrupti aliquam ut sit assumenda aspernatur consequuntur. Ut est ullam iusto facilis voluptatibus. Sit est cum quos. Quasi deleniti non fugit iste alias consequuntur. Ullam ad ut culpa est reiciendis molestiae. Reiciendis ab veritatis a totam inventore nihil voluptatem occaecati. Quisquam atque odit quia nam. Laboriosam rem et ut. Maxime qui voluptatem voluptatem. Quos delectus veritatis est doloribus dolor. Odit fugit omnis magni ipsam quia rem aut. Et alias sint non. Consequuntur dignissimos veritatis debitis corporis esse. Quaerat voluptatem unde aut. Iusto laborum omnis quis amet atque. Sint culpa delectus non soluta temporibus saepe. Sequi saepe corrupti aliquam ut sit assumenda aspernatur consequuntur. Ut est ullam iusto facilis voluptatibus. Sit est cum quos. Quasi deleniti non fugit iste alias consequuntur. Ullam ad ut culpa est reiciendis molestiae. Reiciendis ab veritatis a totam inventore nihil voluptatem occaecati. Quisquam atque odit quia nam. Laboriosam rem et ut. Maxime qui voluptatem voluptatem. Quos delectus veritatis est doloribus dolor. Odit fugit omnis magni ipsam quia rem aut. Et alias sint non. Consequuntur dignissimos veritatis debitis corporis esse. Quaerat voluptatem unde aut. Iusto laborum omnis quis amet atque. Sint culpa delectus non soluta temporibus saepe. Sequi saepe corrupti aliquam ut sit assumenda aspernatur consequuntur. Ut est ullam iusto facilis voluptatibus. Sit est cum quos. Quasi deleniti non fugit iste alias consequuntur. Ullam ad ut culpa est reiciendis molestiae. Reiciendis ab veritatis a totam inventore nihil voluptatem occaecati. Quisquam atque odit quia nam. Laboriosam rem et ut. Maxime qui voluptatem voluptatem. Quos delectus veritatis est doloribus dolor. Odit fugit omnis magni ipsam quia rem aut. Et alias sint non. Consequuntur dignissimos veritatis debitis corporis esse. Quaerat voluptatem unde aut. Iusto laborum omnis quis amet atque. Sint culpa delectus non soluta temporibus saepe. Sequi saepe corrupti aliquam ut sit assumenda aspernatur consequuntur. Ut est ullam iusto facilis voluptatibus. Sit est cum quos. Quasi deleniti non fugit iste alias consequuntur. Ullam ad ut culpa est reiciendis molestiae. Reiciendis ab veritatis a totam inventore nihil voluptatem occaecati. Quisquam atque odit quia nam. Laboriosam rem et ut. Maxime qui voluptatem voluptatem.',
bottomContent: 'bottomContent' as unknown as JSX.Element,
onCancel: 'withCallback' as unknown as () => void,
onBackClick: 'withCallback' as unknown as () => void,
Expand Down
142 changes: 74 additions & 68 deletions packages/components/src/components/NewModal/NewModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import {
borders,
Elevation,
mapElevationToBackground,
mapElevationToBackgroundToken,
prevElevation,
spacingsPx,
} from '@trezor/theme';

import { IconButton } from '../buttons/IconButton/IconButton';
import { Text } from '../typography/Text/Text';
import { H3 } from '../typography/Heading/Heading';
import { ElevationContext } from '../ElevationContext/ElevationContext';
import { ElevationContext, useElevation } from '../ElevationContext/ElevationContext';
import { useScrollShadow } from '../../utils/useScrollShadow';
import { NewModalButton } from './NewModalButton';
import { NewModalContext } from './NewModalContext';
Expand Down Expand Up @@ -131,7 +131,7 @@ type NewModalProps = AllowedFrameProps & {
'data-testid'?: string;
};

const NewModalBase = ({
const _NewModalBase = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like this naming:D There must be something better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to come up with better name / rename it later ;)

children,
variant = 'primary',
size = 'medium',
Expand All @@ -147,7 +147,9 @@ const NewModalBase = ({
const frameProps = pickAndPrepareFrameProps(rest, allowedNewModalFrameProps);
const { scrollElementRef, onScroll, ShadowContainer, ShadowTop, ShadowBottom } =
useScrollShadow();
const modalBackgroundColor = mapElevationToBackgroundToken({ $elevation: MODAL_ELEVATION });

const { elevation } = useElevation();

const hasHeader = onBackClick || onCancel || heading || description;

useEvent('keydown', (e: KeyboardEvent) => {
Expand All @@ -157,74 +159,78 @@ const NewModalBase = ({
});

return (
<ElevationContext baseElevation={MODAL_ELEVATION}>
<NewModalContext.Provider value={{ variant }}>
<Container
$elevation={MODAL_ELEVATION}
$size={size}
onClick={e => e.stopPropagation()}
data-testid={dataTest}
{...frameProps}
>
{hasHeader && (
<Header>
{onBackClick && (
<IconButton
variant="tertiary"
icon="caretLeft"
data-testid="@modal/back-button"
onClick={onBackClick}
size="small"
/>
)}

<HeadingContainer>
{heading && <Heading>{heading}</Heading>}
{description && (
<Text variant="tertiary" typographyStyle="hint">
{description}
</Text>
)}
</HeadingContainer>

{onCancel && (
<IconButton
variant="tertiary"
icon="close"
data-testid="@modal/close-button"
onClick={onCancel}
size="small"
/>
)}
</Header>
<Container
$elevation={elevation}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$size={size}
onClick={e => e.stopPropagation()}
data-testid={dataTest}
{...frameProps}
>
{hasHeader && (
<Header>
{onBackClick && (
<IconButton
variant="tertiary"
icon="caretLeft"
data-testid="@modal/back-button"
onClick={onBackClick}
size="small"
/>
)}
<ShadowContainer>
<ShadowTop backgroundColor={modalBackgroundColor} />
<ScrollContainer onScroll={onScroll} ref={scrollElementRef}>
<Body id={NEW_MODAL_CONTENT_ID}>
{icon && (
<IconWrapper
$variant={variant}
$size={ICON_SIZE}
$isPushedTop={
!!onCancel && !heading && !description && !onBackClick
}
>
<Icon name={icon} size={ICON_SIZE} variant={variant} />
</IconWrapper>
)}
{children}
</Body>
</ScrollContainer>
<ShadowBottom backgroundColor={modalBackgroundColor} />
Copy link
Contributor Author

@peter-sanderson peter-sanderson Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 ☝️

image

</ShadowContainer>
{bottomContent && <Footer>{bottomContent}</Footer>}
</Container>
</NewModalContext.Provider>
</ElevationContext>

<HeadingContainer>
{heading && <Heading>{heading}</Heading>}
{description && (
<Text variant="tertiary" typographyStyle="hint">
{description}
</Text>
)}
</HeadingContainer>

{onCancel && (
<IconButton
variant="tertiary"
icon="close"
data-testid="@modal/close-button"
onClick={onCancel}
size="small"
/>
)}
</Header>
)}
<ShadowContainer>
<ShadowTop />
<ScrollContainer onScroll={onScroll} ref={scrollElementRef}>
<Body id={NEW_MODAL_CONTENT_ID}>
{icon && (
<IconWrapper
$variant={variant}
$size={ICON_SIZE}
$isPushedTop={
!!onCancel && !heading && !description && !onBackClick
}
>
<Icon name={icon} size={ICON_SIZE} variant={variant} />
</IconWrapper>
)}
{children}
</Body>
</ScrollContainer>
<ShadowBottom />
</ShadowContainer>
{bottomContent && <Footer>{bottomContent}</Footer>}
</Container>
);
};

const NewModalBase = (props: NewModalProps) => (
<ElevationContext baseElevation={prevElevation[MODAL_ELEVATION]}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<NewModalContext.Provider value={{ variant: props.variant }}>
<_NewModalBase {...props} />
</NewModalContext.Provider>
</ElevationContext>
);

const NewModal = (props: NewModalProps) => {
const { alignment, onCancel } = props;

Expand Down
Loading