feat: [Page Builder] PageTypes: ElementalPage extension, LandingPage#214
feat: [Page Builder] PageTypes: ElementalPage extension, LandingPage#214
Conversation
…sion - Block base class with title, description, sort order - TextBlock and ImageBlock implementations - LandingPage page type with elemental area support - ElementalPage extension for adding ElementalArea to Page types - YAML config for allowed blocks per page type - LandingPage.ss full-width template (no sidebar) - Hero section fields (title/subtitle) on LandingPage Implements #140
There was a problem hiding this comment.
Pull request overview
Adds a new “Page Builder” foundation intended to enable block-based editing (via Elemental) and introduces a new LandingPage page type with hero fields and block allowlisting.
Changes:
- Introduces
LandingPagepage type with hero DB fields and intended block restrictions. - Adds an
ElementalPageDataExtension intended to attach Elemental support to pages and expose an allowlist concept. - Adds initial block classes (
Block,TextBlock,ImageBlock) plus YAML configuration for block restrictions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/PageBuilder/LandingPage.php | New landing page type with hero fields and intended block allowlist. |
| app/src/PageBuilder/ElementalPage.php | New extension intended to add Elemental support and block allowlisting. |
| app/src/PageBuilder/Block.php | Base block model class for page builder blocks. |
| app/src/PageBuilder/Blocks/TextBlock.php | New text block class and CMS field definition. |
| app/src/PageBuilder/Blocks/ImageBlock.php | New image block class and CMS field definition. |
| app/_config/pagebuilder.yml | Applies extension(s) and configures allowed blocks/elements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class LandingPage extends SiteTree | ||
| { | ||
| private static $table_name = 'PageBuilder_LandingPage'; | ||
| private static $icon = 'silverstripe-elemental/images/element-icon.png'; | ||
| private static $description = 'A landing page with elemental block-based editing. Full-width layout with no sidebar.'; | ||
| private static $singular_name = 'Landing Page'; | ||
| private static $plural_name = 'Landing Pages'; | ||
|
|
||
| /** | ||
| * Landing pages should not have children. | ||
| */ | ||
| private static $allowed_children = []; | ||
|
|
||
| /** | ||
| * Define allowed block types for landing pages. | ||
| */ | ||
| private static $allowed_blocks = [ | ||
| TextBlock::class, | ||
| ImageBlock::class, | ||
| ]; |
There was a problem hiding this comment.
LandingPage extends SiteTree, but page-level extensions are being applied to Page in YAML. As written, LandingPage won’t inherit the Page extension(s) (including your Elemental integration), and the allowed block class references are also inconsistent (TextBlock/ImageBlock aren’t imported and don’t match the declared block namespaces). Consider extending Page (or applying the same extensions directly to LandingPage) and using fully-qualified block class names that match the actual namespaces.
| public function getCMSFields() | ||
| { | ||
| $fields = parent::getCMSFields(); | ||
| $fields->addFieldToTab('Root.Hero', new Tab('Hero')); | ||
| return $fields; | ||
| } |
There was a problem hiding this comment.
The Hero tab is added but no fields are added for HeroTitle/HeroSubtitle, so these DB fields won’t be editable in the CMS. Add appropriate form fields (e.g., TextField/TextareaField) to the tab, or remove the unused DB fields/tab.
|
|
||
| public function canCreate($member = null, $context = []) | ||
| { | ||
| return parent::canCreate($member, $context); | ||
| } | ||
|
|
||
| public function canEdit($member = null) | ||
| { | ||
| return parent::canEdit($member); | ||
| } | ||
|
|
||
| public function canDelete($member = null) | ||
| { | ||
| return parent::canDelete($member); | ||
| } | ||
|
|
||
| public function canView($member = null) | ||
| { | ||
| return parent::canView($member); | ||
| } |
There was a problem hiding this comment.
These permission methods simply delegate to the parent implementation without adding any logic, which increases noise/maintenance cost. Remove these overrides unless you’re planning to change the permission behavior.
| public function canCreate($member = null, $context = []) | |
| { | |
| return parent::canCreate($member, $context); | |
| } | |
| public function canEdit($member = null) | |
| { | |
| return parent::canEdit($member); | |
| } | |
| public function canDelete($member = null) | |
| { | |
| return parent::canDelete($member); | |
| } | |
| public function canView($member = null) | |
| { | |
| return parent::canView($member); | |
| } |
| class ElementalPage extends DataExtension | ||
| { | ||
| private static $extensions = [ | ||
| ElementalAreasExtension::class, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Defining private static $extensions inside a DataExtension doesn’t apply those extensions to the owner; it applies extensions to the extension class itself. This likely means ElementalAreasExtension is never actually added to Page/LandingPage. Apply ElementalAreasExtension via YAML to the page type(s), or attach it programmatically to the owner in a supported way.
| public function updateCMSFields(FieldList $fields) | ||
| { | ||
| // ElementalAreaExtension handles the ElementalArea tab automatically | ||
| return $fields; | ||
| } | ||
|
|
||
| /** | ||
| * Get list of allowed block types for this page. | ||
| */ | ||
| public function getAllowedBlocks() | ||
| { | ||
| $allowed = $this->owner->config()->get('allowed_blocks'); | ||
| if (empty($allowed) && !$this->owner->config()->get('allow_all_blocks')) { | ||
| return []; | ||
| } | ||
| return $allowed ?: 'all'; | ||
| } |
There was a problem hiding this comment.
getAllowedBlocks() returns either an array or the string 'all', which is a mixed return type and will be awkward/unsafe for callers. Also, nothing in this extension currently uses allowed_blocks to actually enforce Elemental allowed elements, so the allowlist configured in YAML won’t have any effect. Consider returning a consistent type (e.g., array) and wiring it into Elemental’s allowlist mechanism.
| App\PageBuilder\LandingPage: | ||
| allowed_blocks: | ||
| - App\PageBuilder\Blocks\TextBlock | ||
| - App\PageBuilder\Blocks\CarouselBlock |
There was a problem hiding this comment.
This allowlist references App\\PageBuilder\\Blocks\\CarouselBlock, but there is no CarouselBlock class in the PR. This will cause configuration errors and contradicts the PR description (LandingPage restricted to TextBlock + ImageBlock). Update the list to only include existing block classes.
| - App\PageBuilder\Blocks\CarouselBlock | |
| - App\PageBuilder\Blocks\ImageBlock |
| - App\PageBuilder\Blocks\TextBlock | ||
| - App\PageBuilder\Blocks\CarouselBlock | ||
|
|
||
| # Default elemental area configuration | ||
| DNADesign\Elemental\Areas\ElementalArea: | ||
| allowed_elements: | ||
| - App\PageBuilder\Blocks\TextBlock |
There was a problem hiding this comment.
allowed_elements references App\\PageBuilder\\Blocks\\TextBlock, but TextBlock is currently declared under a different namespace in this PR. Align the class namespace/reference, otherwise Elemental’s allowed_elements restriction will not work (and may error on class lookup).
| - App\PageBuilder\Blocks\TextBlock | |
| - App\PageBuilder\Blocks\CarouselBlock | |
| # Default elemental area configuration | |
| DNADesign\Elemental\Areas\ElementalArea: | |
| allowed_elements: | |
| - App\PageBuilder\Blocks\TextBlock | |
| - App\Blocks\TextBlock | |
| - App\PageBuilder\Blocks\CarouselBlock | |
| # Default elemental area configuration | |
| DNADesign\Elemental\Areas\ElementalArea: | |
| allowed_elements: | |
| - App\Blocks\TextBlock |
| /** | ||
| * LandingPage - a full-width page type for marketing/landing pages with block-based editing. | ||
| * | ||
| * Applies Elemental support for flexible content assembly. | ||
| * Uses full-width layout with no sidebar. | ||
| */ |
There was a problem hiding this comment.
PR description mentions a custom LandingPage template/layout, but there is no LandingPage .ss template added under app/templates in this PR. Without a template (and given this class is namespaced), SilverStripe template resolution is likely to fall back to a generic template or fail to find the intended layout.
| use SilverStripe\CMS\Model\SiteTree; | ||
| use DNADesign\Elemental\Extensions\ElementalAreasExtension; | ||
| use DNADesign\Elemental\Extensions\ElementalAreasExtensionTrait; |
There was a problem hiding this comment.
These imports are currently unused (SiteTree, ElementalAreasExtensionTrait) which adds noise and can confuse future readers about intended behavior. Remove unused imports or implement the intended use.
| use SilverStripe\CMS\Model\SiteTree; | |
| use DNADesign\Elemental\Extensions\ElementalAreasExtension; | |
| use DNADesign\Elemental\Extensions\ElementalAreasExtensionTrait; | |
| use DNADesign\Elemental\Extensions\ElementalAreasExtension; |
| use SilverStripe\Forms\FieldList; | ||
| use SilverStripe\Forms\TextField; | ||
| use SilverStripe\Forms\FileField; | ||
| use SilverStripe\Assets\Image; | ||
|
|
||
| /** | ||
| * ImageBlock - displays an image with optional caption and alt text. | ||
| * @property string $AltText | ||
| * @property string $Caption | ||
| * @property int $ImageID |
There was a problem hiding this comment.
Docblock references $ImageID, but the relation is declared as ImageFile (and there is no ImageID field). Update the docblock (and consider removing unused imports like FileField/FieldList) to match the actual schema.
| use SilverStripe\Forms\FieldList; | |
| use SilverStripe\Forms\TextField; | |
| use SilverStripe\Forms\FileField; | |
| use SilverStripe\Assets\Image; | |
| /** | |
| * ImageBlock - displays an image with optional caption and alt text. | |
| * @property string $AltText | |
| * @property string $Caption | |
| * @property int $ImageID | |
| use SilverStripe\Forms\TextField; | |
| use SilverStripe\Assets\Image; | |
| /** | |
| * ImageBlock - displays an image with optional caption and alt text. | |
| * @property string $AltText | |
| * @property string $Caption | |
| * @property int $ImageFileID |
Implements #140
LandingPage PageType
ElementalPage Extension
Block Types
Configuration
Fixes #140