Skip to content

feat: add optional pinned prop to content header #108

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

Open
Jay-Topher wants to merge 1 commit into
base: main
Choose a base branch
from
+53 −3
Open

feat: add optional pinned prop to content header #108

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
7 changes: 7 additions & 0 deletions src/lib/sections/ContentSection/ContentSection.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ section.content-section {
.content-section__header {
padding-top: 0.051rem;
margin-bottom: $input-margin-bottom;

&.is-pinned {
position: sticky;
top: 0;
background: $color-x-light;
z-index: 2; // Ensure title displays above notification bar when scrolled
}
}

.content-section__footer {
Expand Down
15 changes: 15 additions & 0 deletions src/lib/sections/ContentSection/ContentSection.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,21 @@ export const WithToolbar = {
},
};

export const WithPinnedHeader = {
args: {
children: (
<>
<ContentSection.Header isPinned>
<MainToolbarExample.render />
</ContentSection.Header>
<ContentSection.Content>
<WithInputGroup.render />
</ContentSection.Content>
</>
)
}
}

export const WithNestedFooter = {
args: {
children: (
Expand Down
13 changes: 13 additions & 0 deletions src/lib/sections/ContentSection/ContentSection.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,16 @@ it("renders custom element for the title", () => {
screen.getByRole("heading", { level: 5, name: "Test Title" }),
).toBeInTheDocument();
});

it("adds appropriate classnames for a pinned header", () => {
render(
<ContentSection>
<ContentSection.Header isPinned>
<ContentSection.Title>Test Title</ContentSection.Title>
</ContentSection.Header>
<ContentSection.Content>Test Content</ContentSection.Content>
<ContentSection.Footer>Test Footer</ContentSection.Footer>
</ContentSection>,
);
expect(document.querySelector(".is-pinned")).toBeInTheDocument();
});
21 changes: 18 additions & 3 deletions src/lib/sections/ContentSection/ContentSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import { AsProp } from "@/types";
interface CommonContentSectionProps extends React.PropsWithChildren {
className?: string;
}

interface ContentSectionHeaderProps extends CommonContentSectionProps {
isPinned?: boolean;
}
export interface ContentSectionProps
extends CommonContentSectionProps,
React.HTMLAttributes<HTMLElement>,
Expand All @@ -18,7 +22,8 @@ export interface ContentSectionProps
/**
* A content section layout component for one of the primary content areas (e.g. main or sidebar).
*
* `ContentSection` has three child components:
* `ContentSection` has four child components:
* - `ContentSection.Header`
* - `ContentSection.Title`
* - `ContentSection.Content`
* - `ContentSection.Footer`
Expand Down Expand Up @@ -57,8 +62,18 @@ const Title = ({ children, className, as, ...props }: ContentSectionProps) => {
);
};

const Header = ({ children, className }: CommonContentSectionProps) => (
<header className={classNames("content-section__header", className)}>
const Header = ({
children,
className,
isPinned,
}: ContentSectionHeaderProps) => (
<header
className={classNames(
"content-section__header",
{ "is-pinned": isPinned },
Copy link

@petermakowski petermakowski Feb 22, 2024

Choose a reason for hiding this comment

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

TLDR; Let's put this on hold while we investigate how to resolve this.

Sorry, I know this is what I've suggested, but I've given this more thought and think we should simply make the header pinned by default (just as the footer is).

Introducing a boolean prop could make this confusing and will be inconsistent with the footer defaults. I went through all instances in MAAS UI and do not see any disadvantages of using the sticky header in all places where this component is used.

One exception is mobile - sticky header doesn't work well when there's a lot of content on mobile as seen below.

Google Chrome screenshot 001577@2x

I'd risk making the header sticky on large breakpoint only, but this could further complicate things.

In case we go ahead with it we should make sure we update the docs as well. Meaning - change from ContentSection.Footer is made sticky by default. to Both ContentSection.Header and ContentSection.Footer are made sticky by default.

className,
)}
>
{children}
</header>
);
Expand Down