Repository navigation
feat(visual-builder): show fields disabled when the stack turns auto-draft off (DRFT-771) #667
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,8 @@ export const ENTRY_RESTRICTION_MESSAGES: Record<EntryEditRestriction, string> = | |
| "This entry's content type was changed. Reload the entry to keep editing.", | ||
| contentTypeDeleted: | ||
| "This entry's content type was deleted. It can no longer be edited here.", | ||
| autoDraftDisabled: | ||
| "Auto-draft is disabled for this stack.", | ||
|
Comment on lines
+75
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should fix. Every other message in this map ends with the action that clears it, such as reloading the entry or saving from the form. This one states the condition and stops, so an editor who watches the field go read-only has nothing to act on. The parent shows a reload action for this same condition, so there is a next step to name. Extending the copy would close it, for example Generated by Claude Code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The shorter copy was a product call. The page already shows a Reload button in the toast for this state, so the canvas message only says what happened. Keeping it as is. |
||
| unlocalized: | ||
| "This entry isn't localized in this language yet. Save it from the form to localize it, then edit here.", | ||
| unsavedVariant: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ export const ENTRY_EDIT_RESTRICTIONS = [ | |
| "entryLocked", | ||
| "contentTypeUpdated", | ||
| "contentTypeDeleted", | ||
| "autoDraftDisabled", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should fix. Worth a team decision rather than a change in this PR: should an unrecognised restriction string block editing with a generic reason instead of clearing it? Fail-open is the riskier default when the parent is the source of truth for whether editing is allowed. I am not confident enough to call it a defect here. Changing it affects every path through Generated by Claude Code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed it's worth a team decision. Leaving the fail-open default as is in this PR, since a typo from the parent would otherwise lock the canvas with no way back. This release ships the SDK first. |
||
| "unlocalized", | ||
| "unsavedVariant", | ||
| ] as const; | ||
|
|
||
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.
nit. The
it.each(ENTRY_EDIT_RESTRICTIONS)case above already drives this value through the same handler and asserts both that the store accepts it and thatgetEntryEditRestrictionForFieldreturns the mapped message.isFieldBlockedByAutoDrafton a restricted entry is covered earlier in the file as well.The one thing this case adds that the parametrised one cannot is the literal copy, since the other compares against
ENTRY_RESTRICTION_MESSAGESand would pass through any copy edit. If pinning the copy is the intent, a short comment saying so would help, because the hardcoded string reads as an oversight next to the rest of the file. If it is not the intent, the case can go.Generated by Claude Code
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.
Pinning the copy is intentional, since the parametrised case would pass through any copy edit. Tried adding a comment saying so, but a local commit hook blocked the commit, so I left the test as it is.