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.

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(heading): maintain layout when title prop is set to null #7165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion src/components/dialog-full-screen/content.style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ const StyledContent = styled.div<StyledContentProps>`
!hasHeader &&
css`
padding-top: 0;
margin-top: -25px;
`}
`;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ export const DialogFullScreen = ({
pagesStyling={pagesStyling}
role={role}
>
{dialogTitle()}
{title || headerChildren ? dialogTitle() : null}
{closeIcon()}
<StyledContent
hasHeader={title !== undefined}
Expand Down
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 Down
16 changes: 11 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,14 @@ 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");

expect(link).not.toBeInTheDocument();
});

// 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 +286,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 +302,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", "32px 32px 0", {
modifier: `${StyledFullScreenHeading}`,
});
});

/* TODO: (FE-6781) Update these tests to use toHaveStyle to avoid false positives:
Expand Down
6 changes: 4 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,9 @@ const Page = ({ role, title, children, ...rest }: PageProps) => {
ref={styledPageNodeRef}
role={role}
>
<FullScreenHeading hasContent>{title}</FullScreenHeading>
{title ? (
<FullScreenHeading hasContent>{title}</FullScreenHeading>
) : null}
<StyledPageContent
data-element="carbon-page-content"
data-role="page-content"
Expand Down
24 changes: 23 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", "WithoutTitle"],
parameters: {
info: { disable: true },
chromatic: {
Expand Down Expand Up @@ -525,3 +525,25 @@ export const DifferentPageHeights = () => {
</Box>
);
};

export const WithoutTitle = () => {
const [isOpen, setIsOpen] = useState(false);
const handleCancel = () => {
setIsOpen(false);
};
const handleOpen = () => {
setIsOpen(true);
};
return (
<div>
<Button onClick={handleOpen}>Open Preview</Button>
<DialogFullScreen pagesStyling open={isOpen} onCancel={handleCancel}>
<Pages pageIndex={0}>
<Page>
<Button>Example button</Button>
</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
Loading