fix: avoid necessity for overwriting height and scrolling behavior [CFISO-1861]

[Lelith]

Purpose of PR

This is a refactor / rewrite of the layout component structure as well as it's styling. It forces the main container, which gives the white background always to be the height of the current view height - offset from the navigation.

The offset will be adjusted for the content height based on wether or not the component contains a header.

The scrolling behavior changes from beeing on the outer wrapper to the individual content elements as soon as at least one sidebar is available. To make all three different areas individually scrollable.

This also extended the storybook example for this component to show different variations with, without header and with different amounts of content.

variant narrow, header and right sidebar

variant wide header, no sidebars

variant wide, header and both sidebars

variant fullscreen, header and both sidebars

[changeset-bot]

⚠️ No Changeset found

Latest commit: d2a030c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR

Some errors occurred when validating the changesets config:
The package "@contentful/f36-components" depends on the ignored package "@contentful/f36-avatar", but "@contentful/f36-components" is not being ignored. Please add "@contentful/f36-components" to the `ignore` option.
The package "@contentful/f36-components" depends on the ignored package "@contentful/f36-image", but "@contentful/f36-components" is not being ignored. Please add "@contentful/f36-components" to the `ignore` option.
The package "@contentful/f36-components" depends on the ignored package "@contentful/f36-header", but "@contentful/f36-components" is not being ignored. Please add "@contentful/f36-components" to the `ignore` option.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
forma-36	✅ Ready (Inspect)	Visit Preview	Sep 27, 2024 11:58am

[damann]

Thanks a bunch for this, Kathrin!
Please consider that sidebars should always be tugged to the side of the page cut. Only the content becomes narrow. At least that was the intent.

[Lelith]

Thanks a bunch for this, Kathrin! Please consider that sidebars should always be tugged to the side of the page cut. Only the content becomes narrow. At least that was the intent.

@damann ah, i thought we might want to change that, because we got the feedback that it looked super weird when the content became scrollable and it looked like the gap is unintentional. but we can play around with it

[denkristoffer]

Thanks a bunch for this, Kathrin! Please consider that sidebars should always be tugged to the side of the page cut. Only the content becomes narrow. At least that was the intent.

@damann ah, i thought we might want to change that, because we got the feedback that it looked super weird when the content became scrollable and it looked like the gap is unintentional.

The gap should still be scrollable, meaning you can hover it and scroll the content from there, so it's "part of the content but not really". It's one of the lovely challenges we face due to the way we handle overflow in the web app 😅

[damann]

Not quite sure what the scrolling has to do with the gap. Can we discuss this in power hour, please?

[wiz-inc-38d59fb8d7]

Wiz Scan Summary

Scan Module						Total
IaC Misconfigurations	0	0	0	0	0	0
Vulnerabilities	0	0	0	0	0	0
Sensitive Data	0	0	0	0	0	0
Secrets	0	0	0	0	0	0
Total	0	0	0	0	0	0

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

[cf-remylenoir]

It looks good to me, thank you for doing this work!

---

Something not introduced by your changes but that we should fix (can be in a follow-up): we violate a11y landmarks because we are nesting the <header> tag.

See the permitted parents.

[denkristoffer]

The changes don't work with the visual modeler: https://github.com/contentful/visual-modeler/pull/459

Now, I'm pretty sure this works fine once the VM goes into the web app, but it seems like there's an expectation now that the Layout is at a minimum always rendered with a navbar?

[Lelith]

@denkristoffer yeah, the layout is now only to be able to be used as the outer wrapper for content in the user_interface with a navbar. otherwise its impossible to pair the sidebars, scrolling behavior AND stretching it always to the bottom of the page. Its no longer a generic "3 columns and a header" component.

to make it more flexible we could offer a offset prop so that the default offset is the navbar or you can overwrite it.

[denkristoffer]

to make it more flexible we could offer a offset prop so that the default offset is the navbar or you can overwrite it.

@Lelith Yes I was thinking similar thoughts, but I would probably default to no offset and then we can wrap the layout once in a component in the web app. I am considering use by customers, out of the box without having to change settings.

[denkristoffer]

How does this work at the moment on pages with the fullscreen behaviour and no nav (Like live preview)? Are they fine?

[cf-remylenoir]

How does this work at the moment on pages with the fullscreen behavior and no nav (Like live preview)? Are they fine?

It looks fine, but other views lack padding at the bottom and their large vertical content is cut.
I am looking at whether it comes from the changes here or the styling override in the web app.

EDIT: I was missing the offset value, hence the lack of padding mentioned above.

[massao]

LGTM

[cf-remylenoir]

LGTM

Thanks for the review, after discussing with Daniel about the sidebar widths, I am working on updating the API to have various sizes (280px and 340px). The default is 280px. I will push the changes soon.