-
Notifications
You must be signed in to change notification settings - Fork 18
PRO-7846: Layout widget #82
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
- Introduced '@apostrophecms/layout' widget in home and default page areas. - Updated layout styles to allow for a wider main content area. - Improved HTML structure for better layout management.
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 quite get the second area on the default page. Why break video out to a standalone area?
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 know this is only a draft and I am reviewing prematurely so this is a comment, not a blocker.
I think you should use 12 steps, and also explicitly configure the column widget's allowed contents even if it's the same as the default, so that it is easier for developers to understand how to add widgets to it. Otherwise layout feels like a black box and people reading Bob's documentation site won't really know what to do to add their very first custom widget.
| options: { | ||
| widgets: { | ||
| '@apostrophecms/layout': { | ||
| columns: 6, |
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.
Use 12
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.
Not appropriate here, it's tested and feels right in the context of the space it has.
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.
The Default page has 12. This also showcases the option override.
| @@ -0,0 +1,18 @@ | |||
| // Modify the layout column widget to allow the desired content types | |||
| // in the Layout widget. | |||
| export default { | |||
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.
@boutell Explicit define so that it's clear how to modify.
@BoDonkey It's a visual thing in the diff view. The widget list is |
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 allows only one level of layouts. Please demo nested layouts in a way that doesn't allow infinite nesting, and does restrict steps in a nested layout. (This means a subclass is needed.)
See the nested-layout branch for a working example although not in the lineage of this PR.
DO NOT MERGE UNTIL AFTER RELEASE