Skip to content

Commit

Permalink
fix(heading): maintain layout when title prop is set to null
Browse files Browse the repository at this point in the history
  • Loading branch information
DobroTora committed Jan 31, 2025
1 parent ea849e3 commit 4164840
Show file tree
Hide file tree
Showing 7 changed files with 120 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ export const DialogFullScreen = ({
pagesStyling={pagesStyling}
role={role}
>
{dialogTitle()}
{/* Conditionally render the dialog title */}
{title || headerChildren ? dialogTitle() : null}
{closeIcon()}
<StyledContent
hasHeader={title !== undefined}
Expand Down
5 changes: 3 additions & 2 deletions src/components/dialog-full-screen/dialog-full-screen.style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ const StyledDialogFullScreen = styled.div<{ pagesStyling?: boolean }>`
css`
${StyledContent} {
padding: 0;
margin-top: -25px;
}
${StyledIconButton} {
Expand All @@ -58,8 +57,10 @@ const StyledDialogFullScreen = styled.div<{ pagesStyling?: boolean }>`
z-index: 1;
}
/* If the component has a title, we need to keep the same styles. If it does not have a title,
we need the padding to compensate for loss of 32px height */
${StyledFullScreenHeading} {
padding: 32px 32px 0;
padding: 64px 32px 0;
}
${StyledHeading} {
Expand Down
17 changes: 12 additions & 5 deletions src/components/dialog-full-screen/dialog-full-screen.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,15 @@ test("renders the element given in the `headerChildren` prop", () => {
).toBeVisible();
});

test("does not render anything if no `headerChildren` prop is provided", () => {
render(<DialogFullScreen open />);

const link = screen.queryByRole("link");

/*eslint-disable */
expect(link).toBeNull();
});

// test here for coverage only - disableContentPadding prop already tested in both Playwright and Chromatic
test("padding is removed from the content when the `disableContentPadding` prop is passed", () => {
render(
Expand Down Expand Up @@ -278,11 +287,6 @@ test("applies the appropriate styles when the `pagesStyling` prop is set", () =>
expect(dialog).toHaveStyleRule("z-index", "1", {
modifier: `${StyledIconButton}`,
});

expect(dialog).toHaveStyleRule("padding", "32px 32px 0", {
modifier: `${StyledFullScreenHeading}`,
});

expect(dialog).toHaveStyleRule("width", "auto", {
modifier: `${StyledHeading}`,
});
Expand All @@ -299,6 +303,9 @@ test("applies the appropriate styles when the `pagesStyling` prop is set", () =>
expect(dialog).toHaveStyleRule("margin", "0 0 0 3px", {
modifier: `${StyledHeading} ${StyledHeader}`,
});
expect(dialog).toHaveStyleRule("padding", "64px 32px 0", {
modifier: `${StyledFullScreenHeading}`,
});
});

/* TODO: (FE-6781) Update these tests to use toHaveStyle to avoid false positives:
Expand Down
3 changes: 1 addition & 2 deletions src/components/heading/heading.style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import addFocusStyling from "../../style/utils/add-focus-styling";

const StyledHeading = styled.div`
width: 100%;
min-height: 53px;
${margin}
`;

Expand Down Expand Up @@ -154,10 +155,8 @@ const StyledDivider = styled(Hr)`

const StyledHeaderHelp = styled(Help)`
display: inline-block;
margin-left: -6px;
margin-right: 16px;
position: relative;
top: -4px;
height: 22px;
`;

Expand Down
7 changes: 5 additions & 2 deletions src/components/pages/page/page.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { filterStyledSystemPaddingProps } from "../../../style/utils";

export interface PageProps extends PaddingProps {
/** The title for the page, normally a Heading component. */
title: React.ReactNode;
title?: React.ReactNode;
/** This component supports children. */
children: React.ReactNode;
/** The ARIA role to be applied to the component */
Expand Down Expand Up @@ -38,7 +38,10 @@ const Page = ({ role, title, children, ...rest }: PageProps) => {
ref={styledPageNodeRef}
role={role}
>
<FullScreenHeading hasContent>{title}</FullScreenHeading>
{/* Conditionally rendered the heading */}
{title ? (
<FullScreenHeading hasContent>{title}</FullScreenHeading>
) : null}
<StyledPageContent
data-element="carbon-page-content"
data-role="page-content"
Expand Down
88 changes: 87 additions & 1 deletion src/components/pages/pages-test.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Box from "../box";

export default {
title: "Pages/Test",
includeStories: ["DefaultStory", "DifferentPageHeights"],
includeStories: ["DefaultStory", "DifferentPageHeights", "TEST_TEST"],
parameters: {
info: { disable: true },
chromatic: {
Expand Down Expand Up @@ -525,3 +525,89 @@ export const DifferentPageHeights = () => {
</Box>
);
};

// Story that demonstrates the example of the Page component not having a title.
// This will need to be given a better name and we must ensure it is captured by Chromatic.
// You'll need to use the `isChromatic` helper that we use in Dialog for this to work.
export const TEST_TEST = ({
initialPageIndex,
}: PageStoryProps & Partial<PageProps>) => {
const [isOpen, setIsOpen] = useState(false);
const [pageIndex, setPageIndex] = useState(Number(initialPageIndex) || 0);
const [isDisabled, setIsDisabled] = useState(false);
const handleCancel = (
ev: React.KeyboardEvent<HTMLElement> | React.MouseEvent<HTMLElement>,
) => {
setIsOpen(false);
setPageIndex(0);
action("cancel")(ev);
};
const handleOpen = (
event: React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>,
) => {
setIsOpen(true);
if (!initialPageIndex) {
setPageIndex(0);
} else setPageIndex(Number(initialPageIndex));
action("open")(event);
};
const handleOnClick = (
ev: React.MouseEvent<HTMLButtonElement | HTMLAnchorElement>,
) => {
setIsDisabled(true);
setPageIndex(pageIndex + 1);
setTimeout(() => {
setIsDisabled(false);
}, 50);
action("click")(ev);
action("slide")(`Page index: ${pageIndex + 1}`);
};
const handleBackClick = (
ev:
| React.MouseEvent<HTMLAnchorElement>
| React.MouseEvent<HTMLButtonElement>
| React.KeyboardEvent<HTMLAnchorElement>
| React.KeyboardEvent<HTMLButtonElement>,
) => {
setIsDisabled(true);
setTimeout(() => {
setIsDisabled(false);
}, 50);
if (!isDisabled) {
ev.preventDefault();
setPageIndex(pageIndex - 1);
action("click")(ev);
action("slide")(`Page index: ${pageIndex + 1}`);
}
};
return (
<div>
<Button onClick={handleOpen}>Open Preview</Button>
<DialogFullScreen pagesStyling open={isOpen} onCancel={handleCancel}>
<Pages initialpageIndex={initialPageIndex} pageIndex={pageIndex}>
<Page>
<Button onClick={handleOnClick} disabled={isDisabled}>
Go to second page
</Button>
</Page>
<Page>
<Button onClick={handleOnClick} disabled={isDisabled}>
Go to third page
</Button>
</Page>
<Page
title={
<Heading
title={null}
backLink={handleBackClick}
divider={false}
/>
}
>
Third Page
</Page>
</Pages>
</DialogFullScreen>
</div>
);
};
10 changes: 10 additions & 0 deletions src/components/pages/pages.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,16 @@ test("navigating to the next page should render the first page when currently on
expect(screen.getByRole("heading", { name: "My First Page" })).toBeVisible();
});

test("component renders correctly with no title prop passed", () => {
render(
<Pages pageIndex={0}>
<Page>First Page</Page>
</Pages>,
);

expect(screen.getByTestId("visible-page")).toHaveTextContent("First Page");
});

test("when attempting to navigate pages and there is only one page, it should not change the rendered content", async () => {
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
const WithSinglePage = () => {
Expand Down

0 comments on commit 4164840

Please sign in to comment.