Repository navigation
Conversation
awcodes
left a comment
There was a problem hiding this comment.
Thanks. The server side is well thought through. Validation on save is the real guarantee, and executeInsertBrick() catches the insert paths that the disabled() check doesn't.
I haven't decided on the feature yet, since this came in before I'd replied in #82. If it goes ahead, it needs:
- Hide Duplicate at the limit. The server already refuses a duplicate past the limit, because
DuplicateBrickActioninserts throughexecuteCommands(), so no server change is needed. But the button still shows, does nothing when clicked, and may push an empty undo step. - Naming.
Mason::getMaxInstances(): arrayshares a name withBrick::getMaxInstances(): ?intbut returns something different. Something likegetBrickLimits()would read more clearly. aria-disabledon the sidebardivs. They have no role, so it isn't announced. Either give them a role or drop it.- Docs. Adding a limit to an existing brick blocks saving any record already over it, until an author removes the extra. That's the right behaviour, but please call it out in the docs.
- Drop the
workbench/resources/dist/changes when you rebase. That directory is git-ignored on 3.x now.
The ->disabled() in BrickAction can also be replaced by any brick's configureBrickAction(), or by a field's brickAction() callback if #92 lands. The server checks still hold, so it's only a UX gap, but it's worth knowing.
Some bricks belong on a page only once, such as a hero or a page meta brick. Bricks can now set a limit per field. The sidebar and the brick picker show how often a limited brick is used, e.g. 1/1, and grey it out once the limit is reached, so it can't be dragged in or picked. The server checks it as well: the brick action can't be mounted to insert another one, the insert command ignores it, and saving fails validation if the content has too many. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d from the sidebar Mason::getMaxInstances() shared its name with Brick::getMaxInstances() but returned the limits of all bricks; it is now getBrickLimits(). The sidebar's bricks are plain drag sources without a role, so aria-disabled was never announced; the screen reader text with the count stays. The docs warn that adding a limit blocks saving content that is already over it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a95357f to
5b8db25
Compare
|
Thanks. Rebased onto 3.x without the workbench dist files. In 5b8db25: the field method is now getBrickLimits() (and brickLimits in the JS), aria-disabled is gone from the sidebar bricks (they're drag sources only, so a role would promise more than they do; the screen-reader count stays), and the docs warn that adding a limit blocks saving content that's already over it. Hiding Duplicate at the limit is ready on top of #85: the preview and the outline drop the control and the editor doesn't send the request, with tests. I'll add it here once #85 is in, or to #85 if this lands first. Noted on ->disabled() being replaceable by configureBrickAction() or brickAction(). |
As proposed in #82: some bricks belong on a page only once, such as a hero or a page meta brick. Disabling duplicate alone isn't enough, since a brick can still be added from the sidebar, the picker or the add line.
The sidebar and the brick picker show how often a limited brick is used, e.g.
0/1(read out as "0 of 1 used"), and grey it out with a hint once the limit is reached, so it can't be dragged in or picked. Unlimited bricks look as before. The server checks it as well: the brick action can't be mounted to insert another one (editing still works),executeInsertBrick()ignores it, and saving fails validation if the content has too many, e.g. from older content or a request made outside the editor. Documented indocs/bricks/creating.md; the workbench hero is limited to one.The limit applies per field. A Mason field in a brick's own form counts only its own bricks, which keeps Mason free of any knowledge about nesting:
getMaxInstances()and leave the brick out of inner fields'bricks()getMaxInstances()aloneWith #85 (duplicate), the server already refuses a copy past the limit, since DuplicateBrickAction inserts through executeCommands(). Hiding the Duplicate control at the limit is ready on top of #85: the preview and the outline drop it for a brick at its limit, and the editor doesn't send the request. I'll add it to whichever of the two lands second.
Tests go through the Livewire action and the insert command, check the validation message in English and Arabic, and the counter in the sidebar and picker. Checked in Chromium and WebKit.
🤖 Generated with Claude Code