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

feat: add optional pinned prop to content header #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jay-Topher
Copy link
Contributor

Done

  • Added optional isPinned prop to Content.Header

QA steps

  • Ensure the component is displayed correctly across all breakpoints
  • Navigate to the content section docs
  • Ensure that the "With Pinned Header" example works as expected

Fixes

Fixes:

Screenshots

Notes

@webteam-app
Copy link
Collaborator

Demo starting at https://maas-react-components-108.demos.haus

<header
className={classNames(
"content-section__header",
{ "is-pinned": isPinned },
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants