Form editor logic copy to x #656
Conversation
WalkthroughThe pull request enhances the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue (4)
23-36: Ensure consistent naming & styling for the "Copy to" button.
It looks great that you've introduced a new button to trigger the copy-to modal. One minor note: in many UI libraries, consistent naming (e.g.,Copy from/Copy to) is helpful for user clarity. Make sure both buttons match in styling (size, color, spacing) for a cohesive UI experience.
120-160: Confirm user instructions and align them with actual workflow.
The modal text states “Select fields to copy the logic from ‘field.name’ to them.” This is correct logically, but you may consider rephrasing for clarity, for example: “Select one or more fields where you want to apply the logic from ‘field.name’.” This might help avoid confusion, especially for non-technical users.
213-221: Consider reusing logic forcopyFromOptionsandcopyToOptions.
Both computed properties filter out the current field. If such logic grows more complex or if you want to unify shared filtering, consider creating a common helper function or computed property that both can rely on.
342-355: Optionally notify users after copying logic.
The methodcopyLogicToFieldsworks well for its intended purpose. For a more seamless UX, consider displaying a short success notification/toast after copying. This helps users confirm the action has completed successfully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue(5 hunks)
🔇 Additional comments (1)
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue (1)
193-194: Data properties for modal control.
Using showCopyToModal and a separate array copyTo is clean and straightforward. Keep watch for potential future expansions (e.g., partial logic copying) and ensure naming remains clear and future-proof.
There was a problem hiding this comment.
Please apply design improvements from: https://github.com/JhumanJ/notionforms/pull/507/commits/964b71b09c714799de1d7175b7f55a7302f61386
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue (2)
61-100: Consider improving modal accessibility.While the modal implementation is clean, consider enhancing accessibility by:
- Adding
aria-labelledbyto connect modal titles with their content- Adding
role="dialog"to the modals- Ensuring focus management when modals open/close
<modal max-width="sm" + role="dialog" + aria-labelledby="copy-from-title" :show="showCopyFormModal" @close="showCopyFormModal = false" > - <h3 class="font-semibold block text-lg"> + <h3 id="copy-from-title" class="font-semibold block text-lg">Also applies to: 103-139
194-202: Add error handling for edge cases in copyToOptions.The computed property should handle potential edge cases:
- Null/undefined fields in form.properties
- Missing name/id properties
copyToOptions() { return this.form.properties .filter((field) => { - return field.id !== this.field.id + return field && field.id && field.id !== this.field.id }) .map((field) => { - return { name: field.name, value: field.id } + return { + name: field.name || `Unnamed Field (${field.id})`, + value: field.id + } }) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/components/open/forms/components/form-logic-components/ConditionEditor.client.vue(1 hunks)client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue(5 hunks)client/components/pages/forms/show/ExtraMenu.vue(1 hunks)client/pages/forms/[slug]/show.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/components/open/forms/components/form-logic-components/ConditionEditor.client.vue
🔇 Additional comments (6)
client/components/pages/forms/show/ExtraMenu.vue (1)
13-17: Good job updating the button component.
Replacing <v-button> with <UButton> along with the new icon and size attributes is consistent with the PR objective of standardized button usage. Ensure the UButton is properly imported or globally registered, and verify the color "white" stands out in a white background context.
client/pages/forms/[slug]/show.vue (2)
33-42: Confirm styling for the "draft" view button.
Using <UButton color="white"> for a possibly white background could obscure the button. Verify contrast or add additional styling so users can clearly see the button.
43-67: Ensure correctness of v-track directives and routes.
- The
v-track.*directives appear functional, but confirm they’re declared and configured properly in the codebase. - Check that
:to="form.share_url"is the intended approach for external links. Ifshare_urlis external, you may consider using a standard anchor tag or ensure the app’s router can handle external URLs.
client/components/open/forms/components/form-logic-components/FormBlockLogicEditor.vue (3)
10-32: LGTM! Well-structured button group implementation.
The button group implementation is clean and consistent, with clear icons and actions.
174-175: LGTM! Well-typed data properties.
The new data properties are appropriately initialized with correct types for their intended use.
323-336: 🛠️ Refactor suggestion
Enhance copyLogicToFields with validation and user feedback.
The method should:
- Validate input before processing
- Provide user feedback for success/failure
- Handle errors gracefully
- Consider performance with multiple deep clones
copyLogicToFields() {
+ if (!this.logic) {
+ this.$emit('error', 'No logic to copy');
+ return;
+ }
+
if (this.copyTo.length) {
+ try {
+ let successCount = 0;
this.copyTo.forEach((fieldId) => {
const targetField = this.form.properties.find(
(property) => property.id === fieldId
)
if (targetField) {
targetField.logic = clonedeep(this.logic)
+ successCount++;
}
})
+ this.$emit('success', `Logic copied to ${successCount} fields`);
+ } catch (error) {
+ this.$emit('error', 'Failed to copy logic: ' + error.message);
+ }
+ } else {
+ this.$emit('warning', 'No target fields selected');
}
this.showCopyToModal = false
this.copyTo = []
},Let's verify the usage of these events in the parent component:
Summary by CodeRabbit
New Features
UI Enhancements
<v-button>to<UButton>across various components for improved styling and functionality.