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

feat(Modal): css and html structure improvements, add footer shadow for layouts #2568

Merged
merged 13 commits into from
Nov 17, 2024
Merged

feat(Modal): css and html structure improvements, add footer shadow for layouts #2568

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5a2dd5f
feat(Modal): move modal content scrolling logic to lay inside layouts…
YossiSaadi Oct 30, 2024
60d7c1d
feat(Modal): css and html structure improvements 7741641864
YossiSaadi Oct 30, 2024
dc8039e
feat(Modal): add footer shadow for basic and side-by-side layouts 771…
YossiSaadi Oct 30, 2024
c321c6e
test: fix tests to fit new context returned value
YossiSaadi Oct 30, 2024
a44aea7
feat(Modal): add footer shadow for media layout 7719009083
YossiSaadi Oct 31, 2024
3fd90e6
feat(Modal): css and html structure improvements 7741641864
YossiSaadi Oct 31, 2024
52e9e9e
feat(ModalMediaLayout): content should be centered by default
YossiSaadi Nov 4, 2024
4b7ac5d
feat(Modal): move RemoveScroll to contain only the modal
YossiSaadi Nov 7, 2024
f52f1fc
feat(Modal): order of top actions should be after children to avoid u…
YossiSaadi Nov 14, 2024
b86c701
feat(Modal): set 100% height for all layouts to stretch to container
YossiSaadi Nov 14, 2024
c6655f6
fix(ModalMedia): do not enforce css for consumer's media children
YossiSaadi Nov 14, 2024
131cf8a
feat(ModalBasicLayout): always apply padding block end
YossiSaadi Nov 14, 2024
019a745
test(Modal): fix test so first focusable is not close
YossiSaadi Nov 14, 2024
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
17 changes: 2 additions & 15 deletions packages/core/src/components/ModalNew/ModalContent/ModalContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,19 @@
import React, { forwardRef, UIEventHandler, useCallback } from "react";
import cx from "classnames";
import React, { forwardRef } from "react";
import { getTestId } from "../../../tests/test-ids-utils";
import { ComponentDefaultTestId } from "../../../tests/constants";
import styles from "./ModalContent.module.scss";
import { ModalContentProps } from "./ModalContent.types";
import { useModal } from "../context/ModalContext";

const ModalContent = forwardRef(
(
{ children, className, id, "data-testid": dataTestId }: ModalContentProps,
ref: React.ForwardedRef<HTMLDivElement>
) => {
const { setContentScrolled } = useModal();

const onScroll: UIEventHandler<HTMLDivElement> = useCallback(
e => {
setContentScrolled(e.currentTarget?.scrollTop > 0);
},
[setContentScrolled]
);

return (
<div
ref={ref}
className={cx(styles.content, className)}
className={className}
id={id}
data-testid={dataTestId || getTestId(ComponentDefaultTestId.MODAL_NEXT_CONTENT, id)}
onScroll={onScroll}
>
{children}
</div>
Expand Down
73 changes: 0 additions & 73 deletions packages/core/src/components/ModalNew/ModalContent/__tests__/ModalContent.test.tsx
View file
Open in desktop
YossiSaadi marked this conversation as resolved.
Show resolved Hide resolved

This file was deleted.

7 changes: 3 additions & 4 deletions packages/core/src/components/ModalNew/ModalHeader/__tests__/ModalHeader.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { render } from "@testing-library/react";
import ModalHeader from "../ModalHeader";
import { Text as TextIcon } from "../../../Icon/Icons";
import { useModal } from "../../context/ModalContext";
import { ModalContextProps } from "../../context/ModalContext.types";

jest.mock("../../context/ModalContext", () => ({
useModal: jest.fn()
Expand All @@ -14,12 +15,10 @@ describe("ModalHeader", () => {
const simpleDescription = "This is a description";
const descriptionIcon = TextIcon;

const useModalMockedReturnedValue = {
const useModalMockedReturnedValue: ModalContextProps = {
modalId: "modal-id",
setTitleId: jest.fn(),
setDescriptionId: jest.fn(),
isContentScrolled: false,
setContentScrolled: jest.fn()
setDescriptionId: jest.fn()
};

beforeEach(() => {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/components/ModalNew/ModalTopActions/ModalTopActions.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
position: absolute;
right: var(--top-actions-spacing);
top: var(--top-actions-spacing);
z-index: 1;
YossiSaadi marked this conversation as resolved.
Show resolved Hide resolved
}
15 changes: 2 additions & 13 deletions packages/core/src/components/ModalNew/context/ModalContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,10 @@
import React, { createContext, useContext, useMemo, useState } from "react";
import React, { createContext, useContext } from "react";
import { ModalContextProps, ModalProviderProps } from "./ModalContext.types";

const ModalContext = createContext<ModalContextProps | undefined>(undefined);

export const ModalProvider = ({ value, children }: ModalProviderProps) => {
const [isContentScrolled, setContentScrolled] = useState<boolean>(false);

const contextValue = useMemo<ModalContextProps>(
() => ({
...value,
isContentScrolled,
setContentScrolled: (newContentScrolled: boolean) => setContentScrolled(newContentScrolled)
}),
[isContentScrolled, value]
);

return <ModalContext.Provider value={contextValue}>{children}</ModalContext.Provider>;
return <ModalContext.Provider value={value}>{children}</ModalContext.Provider>;
};

export const useModal = (): ModalContextProps => {
Expand Down
5 changes: 1 addition & 4 deletions packages/core/src/components/ModalNew/context/ModalContext.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import React from "react";

export interface ModalContextProps extends ModalProviderValue {
isContentScrolled: boolean;
setContentScrolled: (scrolled: boolean) => void;
}
export type ModalContextProps = ModalProviderValue;

export type ModalProviderValue = {
modalId: string;
Expand Down
20 changes: 17 additions & 3 deletions packages/core/src/components/ModalNew/layouts/ModalBasicLayout/ModalBasicLayout.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,29 @@

.header {
width: 100%;
margin-block: var(--spacing-xl) var(--spacing-medium);
margin-block: var(--spacing-xl) var(--spacing-large);
padding-inline-end: calc(var(--top-actions-spacing) + var(--top-actions-width) + var(--spacing-medium));
padding-inline-start: var(--modal-inline-padding);
}

.divider {
flex-shrink: 0;
}

.content {
padding-inline: var(--modal-inline-padding);
/**
* The footer is optional, so we need to check if it exists before applying padding.
* If it exists (or "has" not supported), we apply padding to the content, if it doesn't, we don't apply padding.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Why not use context? Or data attrs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would force the footer to access the context
I wanted to avoid it
but you're basically right, it's probably a better solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the padding to always be visible, as the footer shouldn't be omit

@supports selector(:has(*)) {
&:has(+ *) {
.content {
padding-block-end: var(--spacing-xl);
}
}
}
@supports not selector(:has(*)) {
.content {
padding-block-end: var(--spacing-xl);
}
}
}
35 changes: 21 additions & 14 deletions packages/core/src/components/ModalNew/layouts/ModalBasicLayout/ModalBasicLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,38 @@ import { getTestId } from "../../../../tests/test-ids-utils";
import { ComponentDefaultTestId } from "../../../../tests/constants";
import styles from "./ModalBasicLayout.module.scss";
import { ModalBasicLayoutProps } from "./ModalBasicLayout.types";
import { useModal } from "../../context/ModalContext";
import Flex from "../../../Flex/Flex";
import Divider from "../../../Divider/Divider";
import ModalFooterShadow from "../ModalFooterShadow";
import ModalLayoutScrollableContent from "../ModalLayoutScrollableContent";
import useLayoutScrolledContent from "../useLayoutScrolledContent";

const ModalBasicLayout = forwardRef(
(
{ children, className, id, "data-testid": dataTestId }: ModalBasicLayoutProps,
ref: React.ForwardedRef<HTMLDivElement>
) => {
const { isContentScrolled, onScroll } = useLayoutScrolledContent();
const [header, content] = React.Children.toArray(children);
const { isContentScrolled } = useModal();

return (
<Flex
direction={Flex.directions.COLUMN}
align={Flex.align.START}
ref={ref}
className={cx(styles.layout, className)}
id={id}
data-testid={dataTestId || getTestId(ComponentDefaultTestId.MODAL_NEXT_BASIC_LAYOUT, id)}
>
<div className={styles.header}>{header}</div>
{isContentScrolled && <Divider className={styles.divider} withoutMargin />}
<div className={styles.content}>{content}</div>
</Flex>
<>
<Flex
direction={Flex.directions.COLUMN}
align={Flex.align.START}
ref={ref}
className={cx(styles.layout, className)}
id={id}
data-testid={dataTestId || getTestId(ComponentDefaultTestId.MODAL_NEXT_BASIC_LAYOUT, id)}
>
<div className={styles.header}>{header}</div>
{isContentScrolled && <Divider className={styles.divider} withoutMargin />}
<ModalLayoutScrollableContent onScroll={onScroll} className={styles.content}>
{content}
</ModalLayoutScrollableContent>
</Flex>
{isContentScrolled && <ModalFooterShadow />}
YossiSaadi marked this conversation as resolved.
Show resolved Hide resolved
</>
);
}
);
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/components/ModalNew/layouts/ModalFooterShadow.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.shadowWrapper::after {
content: "";
position: absolute;
width: 100%;
height: 10px;
box-shadow: var(--box-shadow-medium);
}
8 changes: 8 additions & 0 deletions packages/core/src/components/ModalNew/layouts/ModalFooterShadow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from "react";
import styles from "./ModalFooterShadow.module.scss";

const ModalFooterShadow = () => {
return <div className={styles.shadowWrapper} />;
};

export default ModalFooterShadow;
4 changes: 3 additions & 1 deletion ...New/ModalContent/ModalContent.module.scss → .../ModalLayoutScrollableContent.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
@import "~monday-ui-style/dist/mixins";

.content {
padding-block-end: var(--spacing-xl);
width: 100%;
height: 100%;
padding-inline: var(--modal-inline-padding);
overflow: auto;
@include scroller;
}
14 changes: 14 additions & 0 deletions packages/core/src/components/ModalNew/layouts/ModalLayoutScrollableContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import React from "react";
import cx from "classnames";
import styles from "./ModalLayoutScrollableContent.module.scss";
import { ModalLayoutScrollableContentProps } from "./ModalLayoutScrollableContent.types";

const ModalLayoutScrollableContent = ({ onScroll, className, children }: ModalLayoutScrollableContentProps) => {
return (
<div className={cx(styles.content, className)} onScroll={onScroll}>
{children}
</div>
);
};

export default ModalLayoutScrollableContent;
7 changes: 7 additions & 0 deletions packages/core/src/components/ModalNew/layouts/ModalLayoutScrollableContent.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { ReactNode, UIEventHandler } from "react";

export interface ModalLayoutScrollableContentProps {
onScroll?: UIEventHandler<HTMLDivElement>;
className?: string;
children: ReactNode;
}
17 changes: 13 additions & 4 deletions packages/core/src/components/ModalNew/layouts/ModalMedia.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
.media {
align-items: normal;
align-self: normal;
width: 100%;
height: auto;
max-width: 100%;
max-height: 100%;
display: flex;
justify-content: center;
align-items: center;
flex-shrink: 0;
overflow: hidden;
position: relative;

> * {
Copy link
Member

Choose a reason for hiding this comment

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

This is usually bad for performance as css selectors are read from right to left. Any way to avoid it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can just target img and video tags instead

Copy link
Member

Choose a reason for hiding this comment

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

What if it's a Lottie?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how Lottie would behave anyway with the current CSS I applied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably have Lottie as a dev dep for this kind of cases

Copy link
Member

Choose a reason for hiding this comment

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

Dev dep? Why? Plus I feel like that this css is creating a side effect for the users, don't you think? Because they can provide an img element and use some css on that element but you will most likely override their css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Their css should "win"
I'm using 0,1,0 specificity
so if they override it, their css would be the one rendered
also, you suggested in a different PR to apply a predefined css
otherwise the ModalMedia is redundant as ModalContent

Copy link
Member

Choose a reason for hiding this comment

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

I meant predefined css for an image that you provided by a prop, not by children, but this was dropped, no?
But anyway, I mean that targeting elements inside your component is creating a side effect. I don't think that the css is bad, it just should only affect the elements inside your component and not elements provided by the user. Plus you're applying style that may not fit to all elements like object-fit. I think the best combination would be to have an Image/Video components or maybe Media. This along with the ModalMedia will get along well. But probably out of this scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to release the layouts with a solution for that
I cannot leave it with every user doing whatever they want, it would create inconsistency
let's have a talk about it tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the entire code that's affecting the users
I think each case should be examined differently by the user implementing and each implementation would require a bit different css handling
Thanks!

max-width: 100%;
max-height: 100%;
object-fit: contain;
margin: auto;
}
}
14 changes: 10 additions & 4 deletions packages/core/src/components/ModalNew/layouts/ModalMediaLayout/ModalMediaLayout.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,38 @@
overflow: hidden;

.media {
display: flex;
justify-content: center;
align-items: center;
overflow: hidden;
position: relative;
width: 100%;
height: var(--modal-top-media-height, 240px);
flex-shrink: 0;

height: var(--modal-top-media-height, 240px);
@media (min-width: 1280px) {
--modal-top-media-height: 260px;
}

@media (min-width: 1440px) {
--modal-top-media-height: 260px;
}

@media (min-width: 1720px) {
--modal-top-media-height: 320px;
}
}

.header {
width: 100%;
margin-block: var(--spacing-large) var(--spacing-small);
text-align: center;
margin-block: var(--spacing-xl) var(--spacing-small);
padding-inline: var(--modal-inline-padding);
}

.content {
width: 100%;
flex: 1;
align-self: flex-start;
padding-block-end: var(--spacing-large);
text-align: center;
}
}
Loading