-
Notifications
You must be signed in to change notification settings - Fork 4k
Add fullwidth page option #3344
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 11e9f10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
To prevent unnecessary layout shift when resizing the window smaller
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
/* Blocks that can be full width are automatically expanded on full-width pages. | ||
* Ideally we'd rely on the block type to determine if it can be full width, but | ||
* the block's `fullWidth` property does not differentiate between `undefined` and `false`. | ||
* So instead we hardcode a list of blocks that can be full width. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment, why would these blocks differ from others when the page is full-width?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they automatically grow to expand the size. If we didn't do that, the page option would only affect the page's container, and you would still have to make each block full-width individually.
This way we prevent the option of having a wide page with a lot of space on the right, because any fullwidth block (tabs, tables, ...) grows automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still very confusing, and smells very bad that we have an hardcoded set of block types...
I don't have time to deep dive, but yeah it smells fishy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the following mean?
the block's
fullWidth
property does not differentiate betweenundefined
andfalse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's far from perfect and I am very open to better approaches.
What does the following mean?
the block's
fullWidth
property does not differentiate betweenundefined
andfalse
.
I need to set a block's max width based on if its fullWidth
prop:
fullWidth=undefined
: the block cannot expand, so it should always stay narrow.fullWidth=false
: the block can expand, but is not set to expand by the user. By default we do nothing. On wide pages, we should automatically expand it so the user doesn't have to toggle it manually on every single block.fulLWidth=true
: the block is set to expand by the user. We need to expand it.
The problem is this: the API only returns undefined
and true
. If a block is set to expand, the fullWidth
prop is removed entirely instead of being set to false
, even though it's an optional boolean so can be true|false|undefined
.
This means I cannot know on the front-end if I need to show a block narrow (because blocks without fullWidth
should remain narrow) or expand it (because blocks with fullWidth=true
and fullWidth=false
should expand).
Two solutions I see:
- There's probably a non-strict falsy check somewhere that is removing
fullWidth
instead of setting it tofalse
deliberately. If we change that I can read out all three states and remove this hardcoded array. - We can explicitly save each block as
fullWidth=true
when the page is set towide
layout. This way the front-end can just expand whatever block on whichfullWidth
exists, and keep the rest narrow. But we're then actually editing blocks' markup based on the page they're in, that feels strange.
I think 1 is the easiest solution. But I'm also very open to other ideas I haven't thought of yet.
cc @BrettJephson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but I'm concerned that the logic is twisted with different behaviour based on a set of block types.
@BrettJephson how is it done in the editor? any idea on how to make this better?
/* Blocks that can be full width are automatically expanded on full-width pages. | ||
* Ideally we'd rely on the block type to determine if it can be full width, but | ||
* the block's `fullWidth` property does not differentiate between `undefined` and `false`. | ||
* So instead we hardcode a list of blocks that can be full width. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still very confusing, and smells very bad that we have an hardcoded set of block types...
I don't have time to deep dive, but yeah it smells fishy
/* Blocks that can be full width are automatically expanded on full-width pages. | ||
* Ideally we'd rely on the block type to determine if it can be full width, but | ||
* the block's `fullWidth` property does not differentiate between `undefined` and `false`. | ||
* So instead we hardcode a list of blocks that can be full width. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the following mean?
the block's
fullWidth
property does not differentiate betweenundefined
andfalse
.
Now that the page-width option is available, we can remove the hardcoded ID and get GBO ready to use the customization option. Also makes some tiny tweaks to make the full-width option work better with different blocks and the page's loading skeleton.