Form focused mode #640
Form focused mode #640
Conversation
…ed component - Updated OpenCompleteForm.vue to conditionally render OpenFormFocused for forms with 'focused' format. - Introduced OpenFormFocused.vue to handle focused form submissions, including progress bar, navigation buttons, and captcha support. - Enhanced form submission handling and added necessary props for improved functionality. This change enhances user experience by providing a dedicated focused mode for form submissions.
…tency - Added a check for empty previous fields in OpenForm.vue to prevent errors. - Refactored button layout in OpenFormFocused.vue to improve UI structure and added a slot for submit button when on the last page. - Updated label for cover picture input in FormCustomization.vue to reflect context based on form format. - Adjusted styles in FormEditorPreview.vue to ensure responsive behavior and visual clarity when focused. These changes aim to enhance user experience and maintain consistency across form components.
… customization - Updated OpenFormFocused.vue to allow dynamic button text for navigation based on form configuration. - Added focused format options in FieldOptions.vue, enabling customization of previous and next button texts. These changes improve the flexibility and user experience of the form components.
… for navigation buttons - Modified OpenFormFocused.vue to utilize localized text for 'Back' and 'Next' buttons using the $t function. - Added translations for 'Back' and 'Next' buttons in multiple languages (Arabic, English, Spanish, French, Hindi, Japanese, Portuguese, Chinese). These changes enhance the user experience by providing localized navigation options in the form component.
…mprove navigation - Replaced `currentFields` with `currentField` to simplify field access and enhance clarity in the component's logic. - Updated navigation button text to utilize the new `currentField` structure, ensuring dynamic text rendering based on the current form state. - Removed deprecated code related to `currentFields` to clean up the component and improve maintainability. These changes enhance the overall functionality and readability of the OpenFormFocused component.
WalkthroughThe pull request introduces several changes to support a new Changes
Possibly related PRs
Suggested reviewers
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: 6
🧹 Outside diff range and nitpick comments (8)
client/pages/forms/[slug]/index.vue (1)
Line range hint
1-73: Document the new focused format feature.The introduction of the focused format significantly changes the form's layout and behavior. Consider adding:
- Documentation about this new feature
- Usage guidelines in the README
- Examples of when to use focused vs. regular format
Would you like me to help create the documentation or open a GitHub issue for tracking this task?
client/components/open/forms/OpenFormFocused.vue (1)
10-10: Simplify the submit event handler.The
@submit.prevent=""attribute is unnecessary if no handler is provided. You can shorten it to@submit.preventor remove it entirely if not needed.Apply this diff to simplify:
- @submit.prevent="" + @submit.preventclient/components/open/editors/FormatChangeModal.vue (1)
63-63: Add type definitions for better maintainabilityConsider adding TypeScript or JSDoc type definitions for the computed property.
-const removeBlocks = computed(() => form.value.properties.filter(field => removeTypes.includes(field.type))) +/** + * @type {import('vue').ComputedRef<Array<{ id: string, type: string, name: string }>>} + */ +const removeBlocks = computed(() => form.value.properties.filter(field => removeTypes.includes(field.type)))client/pages/forms/create/index.vue (1)
8-8: Add prop type validationConsider adding prop type validation for the form prop in CreateFormBaseModal component.
In CreateFormBaseModal.vue:
+defineProps({ + form: { + type: Object, + required: true + } +})client/components/open/forms/components/form-components/AddFormBlock.vue (1)
94-94: Improve readability of layoutBlocks filterThe filter condition is complex and could benefit from being broken down for better readability.
Consider refactoring to:
-const layoutBlocks = computed(() => Object.values(blocksTypes).filter(block => block.name.startsWith('nf-') && (isFocused.value ? !['nf-page-break','nf-divider','nf-image','nf-code'].includes(block.name) : true))) +const FOCUSED_MODE_EXCLUDED_BLOCKS = ['nf-page-break', 'nf-divider', 'nf-image', 'nf-code'] +const layoutBlocks = computed(() => { + return Object.values(blocksTypes).filter(block => { + const isLayoutBlock = block.name.startsWith('nf-') + const isAllowedInFocusedMode = !isFocused.value || !FOCUSED_MODE_EXCLUDED_BLOCKS.includes(block.name) + return isLayoutBlock && isAllowedInFocusedMode + }) +})client/components/open/forms/components/form-components/FormCustomization.vue (1)
282-286: Consider handling both format cases explicitlyThe current implementation only handles the 'focused' case. Consider handling the 'regular' case explicitly for better maintainability.
const onChangeFormat = (val) => { if (val === 'focused') { showFormatChangeModal.value = true + } else if (val === 'regular') { + form.value.format = val } }client/components/open/forms/OpenCompleteForm.vue (2)
Line range hint
111-155: Consider extracting the submit button template to reduce duplication.The submit button template is identical in both form components. Consider extracting it into a separate component or slot to improve maintainability.
Example refactor:
-<template #submit-btn="{submitForm}"> - <open-form-button - :loading="loading" - :theme="theme" - :color="form.color" - class="mt-2 px-8 mx-1" - :class="submitButtonClass" - @click.prevent="submitForm" - > - {{ form.submit_button_text }} - </open-form-button> -</template>Create a new component
FormSubmitButton.vueand use it in both places:<template #submit-btn="{submitForm}"> <form-submit-button :loading="loading" :theme="theme" :form="form" :submit-button-class="submitButtonClass" @submit="submitForm" /> </template>
228-229: Remove unused FormTimer import.The
FormTimercomponent is imported but not utilized in the template. Consider removing it if it's not needed.-import FormTimer from './FormTimer.vue'Also applies to: 238-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
api/app/Http/Requests/UserFormRequest.php(1 hunks)api/app/Models/Forms/Form.php(2 hunks)api/database/factories/FormFactory.php(1 hunks)api/database/migrations/2024_12_04_163142_add_format_to_forms_table.php(1 hunks)client/components/open/editors/FormatChangeModal.vue(1 hunks)client/components/open/forms/OpenCompleteForm.vue(4 hunks)client/components/open/forms/OpenForm.vue(1 hunks)client/components/open/forms/OpenFormFocused.vue(1 hunks)client/components/open/forms/components/form-components/AddFormBlock.vue(1 hunks)client/components/open/forms/components/form-components/FormCustomization.vue(5 hunks)client/components/open/forms/components/form-components/FormEditorPreview.vue(4 hunks)client/components/open/forms/fields/components/FieldOptions.vue(1 hunks)client/components/pages/forms/create/CreateFormBaseModal.vue(3 hunks)client/components/pages/forms/create/FormFormatModal.vue(1 hunks)client/composables/forms/initForm.js(1 hunks)client/i18n/lang/ar.json(1 hunks)client/i18n/lang/en.json(1 hunks)client/i18n/lang/es.json(1 hunks)client/i18n/lang/fr.json(1 hunks)client/i18n/lang/hi.json(1 hunks)client/i18n/lang/ja.json(1 hunks)client/i18n/lang/pt.json(1 hunks)client/i18n/lang/zh.json(1 hunks)client/pages/forms/[slug]/index.vue(3 hunks)client/pages/forms/create/index.vue(1 hunks)
🔇 Additional comments (26)
client/i18n/lang/zh.json (1)
6-7: LGTM! Translations are accurate and consistent.
The Chinese translations are idiomatic and align with common UI patterns. Note that "返回" is consistently used for "back" across the UI (also used in cameraUpload.goBack).
client/i18n/lang/ja.json (1)
6-7: LGTM! Translations are accurate and consistent.
The Japanese translations are idiomatic and follow standard UI conventions. Note that "戻る" is consistently used for "back" across the UI (also used in cameraUpload.goBack).
client/i18n/lang/ar.json (1)
6-7: LGTM! Translations are accurate and consistent.
The Arabic translations are idiomatic and follow standard UI conventions. Note that "رجوع" is consistently used for "back" across the UI (also used in cameraUpload.goBack).
Since Arabic is RTL (right-to-left), let's verify that the UI components using these translations have proper RTL support:
✅ Verification successful
Based on the search results, I can see that the application has proper RTL support implemented:
- The form layout has an RTL toggle option (
layout_rtlproperty) that controls the text direction - RTL-specific CSS classes and styling are present in the codebase
- The form's direction is properly set using both CSS direction property and HTML dir attribute
- Components like PhoneInput, VSelect, and MatrixInput have RTL-specific styling classes
The Arabic translations are safe to use as the UI properly handles RTL text direction.
Key evidence:
client/pages/forms/[slug]/index.vue: Sets form direction based onlayout_rtlpropertyclient/components/open/forms/OpenCompleteForm.vue: Applies RTL styling and directionclient/tailwind.config.js: Has RTL-specific variants- Multiple components have RTL-specific classes for proper alignment and styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for RTL-related styling and components in the codebase
# Look for RTL-related CSS classes and directives
rg -i 'rtl|direction:|dir=' --type html --type vue --type css
# Look for RTL-aware component usage
rg -i 'v-rtl|rtl-|isRTL|direction' --type vue --type js
Length of output: 177
Script:
#!/bin/bash
# Let's try a different approach to find RTL-related code
# Search for RTL-related patterns in all files
rg -i 'rtl|direction:|dir='
# Search for style-related files that might contain RTL support
fd -e css -e scss -e less -e styled.js
# Look for i18n or locale configuration files
fd -e js -e ts -e json | rg -i "i18n|locale|lang"
Length of output: 67857
Script:
#!/bin/bash
# Let's search for UI component files and styling that might handle RTL
# Look for any styling files
fd -e css -e scss -e less -e styl -e sass
# Look for component files that might contain RTL handling
fd -e jsx -e tsx -e js -e vue
# Search for direction/RTL related code in all files
rg -i "direction|rtl|ltr|right-to-left|left-to-right" --type-add 'frontend:*.{js,jsx,ts,tsx,vue,css,scss,less,styl,sass}'
Length of output: 100918
client/i18n/lang/en.json (1)
6-7: LGTM! Verify translations across all language files.
The English translations for navigation buttons are accurate and properly capitalized.
Let's verify the consistency of these new keys across all language files:
✅ Verification successful
Translation keys 'back' and 'next' are properly defined across all language files
All language files (ar, en, es, fr, hi, ja, pt, zh) have the required translations for both 'back' and 'next' keys under the 'forms' section. Each translation is appropriately localized for its respective language.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'back' and 'next' keys exist in all language files
# and ensure no language file is missing these translations
# Find all language files
echo "Checking translation consistency across language files..."
for lang_file in $(fd -e json . client/i18n/lang/); do
echo "\nChecking $lang_file:"
# Check for back key
jq -r '.forms.back // "MISSING"' "$lang_file"
# Check for next key
jq -r '.forms.next // "MISSING"' "$lang_file"
done
Length of output: 2298
client/i18n/lang/hi.json (1)
6-7: LGTM! Hindi translations are accurate.
The Hindi translations "पिछला" (back) and "अगला" (next) are grammatically correct and properly convey the navigation context.
client/i18n/lang/pt.json (1)
6-7: LGTM! Portuguese translations are accurate.
The Portuguese translations "Voltar" (back) and "Próximo" (next) are correctly spelled with proper accents and follow Brazilian Portuguese conventions.
client/i18n/lang/es.json (1)
6-7: LGTM! Translations are accurate and consistent
The Spanish translations for navigation buttons are correct and follow common web interface conventions:
- "Atrás" is the standard translation for "back"
- "Siguiente" is the standard translation for "next"
client/i18n/lang/fr.json (2)
6-7: LGTM! Translations are accurate and consistent
The French translations for navigation buttons are correct and follow common web interface conventions:
- "Retour" is the standard translation for "back"
- "Suivant" is the standard translation for "next"
6-6: Consider standardizing "back" translations
Note that both forms.back and forms.cameraUpload.goBack are translated as "Retour". While this is correct, you might want to verify if these should be using the same translation key for consistency and maintainability.
Let's check if other language files have the same pattern:
Also applies to: 35-35
client/pages/forms/[slug]/index.vue (1)
64-73: 🛠️ Refactor suggestion
Ensure proper positioning context and responsive behavior.
The absolute positioning with inset-0 requires a positioned ancestor to work correctly. Additionally, on smaller screens, the centered form might be partially hidden or inaccessible.
Let's check if there's a positioned ancestor:
Consider these improvements:
-<div :class="{'absolute inset-0 flex items-center justify-center': isFocused}">
+<div :class="{
+ 'absolute inset-0 flex items-center justify-center min-h-screen overflow-auto': isFocused,
+ 'relative': !isFocused
+}">client/components/pages/forms/create/CreateFormBaseModal.vue (1)
194-194: Verify the initial value of showFormatModal.
The showFormatModal data property is initialized to true, which will display the FormFormatModal immediately upon component mount. Please confirm if this behavior is intentional.
api/database/migrations/2024_12_04_163142_add_format_to_forms_table.php (1)
7-27: Migration looks good.
The migration correctly adds the format column with a default value and provides a proper rollback mechanism.
api/database/factories/FormFactory.php (1)
53-53: Verify consistency of default format value
The default format value 'regular' has been added. Let's ensure this value is consistently used across the codebase.
✅ Verification successful
Default format value 'regular' is consistent with codebase implementation
The verification shows that:
- The format values are properly defined in
Form::FORMATS = ['regular', 'focused'] - The format field is validated against these values in
UserFormRequestusingRule::in(Form::FORMATS) - The default value 'regular' in the factory aligns with the available formats
- No conflicting default values were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of default format value across the codebase
# Search for format-related constants and default values
echo "Searching for format-related constants and default values..."
rg -g '!{*.lock,*.min.*}' -A 2 "FORMATS.*=.*\[.*'regular'.*'focused'.*\]"
# Search for direct usage of format values
echo "Searching for direct usage of format values..."
rg -g '!{*.lock,*.min.*}' "'format'.*=>.*'regular'"
Length of output: 676
Script:
#!/bin/bash
# Let's verify the format validation and default value in migrations and models
# Check migrations for format column definition
echo "Checking migrations for format column..."
rg -g "*.php" "format.*string" api/database/migrations/
# Check for format validation rules
echo "Checking validation rules for format..."
rg -g "*.php" "format.*in:.*regular" api/app/
# Check for any other occurrences of format values
echo "Checking other format value usages..."
rg -g "*.php" -B2 -A2 "'format'" api/app/
Length of output: 18351
client/components/open/editors/FormatChangeModal.vue (1)
65-69: Verify block removal logic
The format change logic removes blocks based on type. Let's verify if these are all the block types that need removal in focused mode.
✅ Verification successful
Let me generate additional scripts to verify the block removal logic in focused mode.
Based on the search results, I can now provide a final verification response:
Block removal logic is correctly implemented
The code correctly removes the following block types in focused mode:
- nf-page-break
- nf-divider
- nf-image
- nf-code
This is verified by:
- The
removeTypesarray definition matches the block types filtered out - The same block types are filtered in the layout blocks selection (
AddFormBlock.vue) - The format change modal only shows when these blocks exist and format is being changed to focused mode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for block type definitions and usages
# Search for block type definitions
echo "Searching for block type definitions..."
ast-grep --pattern 'const $blockTypes = [
$$$
]'
# Search for block type usages in focused mode context
echo "Searching for block type usages in focused mode context..."
rg -g '!{*.lock,*.min.*}' "focused.*format.*type"
Length of output: 65856
Script:
#!/bin/bash
# Search for removeTypes definition and focused mode related code
# Search for removeTypes definition
echo "Searching for removeTypes definition..."
rg -A 5 "removeTypes.*=" client/
# Search for focused mode related code
echo "Searching for focused mode related code..."
rg -A 5 "format.*=.*focused" client/
Length of output: 5410
client/pages/forms/create/index.vue (1)
Line range hint 116-117: Verify form initialization with format property
The form initialization should include the new format property. Let's verify the initialization logic.
✅ Verification successful
Let me try a different search strategy to verify the form initialization.
Let me try one more search to find the form initialization logic.
Based on the initialization code found in client/composables/forms/initForm.js, I can now provide a definitive response:
The format property is already properly handled in form initialization
The initForm function already includes the format property with a default value of "regular" in its initialization:
format: "regular",This property is part of the default form structure under the "Customization" section. No additional verification or changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check form initialization handling of format property
# Search for form initialization logic
echo "Searching for form initialization..."
rg -g '!{*.lock,*.min.*}' "initForm.*format"
# Search for form data structure
echo "Searching for form data structure..."
ast-grep --pattern 'interface $FormData {
$$$
format?: string
$$$
}'
Length of output: 289
Script:
#!/bin/bash
# Search for form initialization and related data structures in the specific file
echo "Checking the specific file content..."
cat client/pages/forms/create/index.vue
# Search for form-related interfaces and types
echo "Searching for form interfaces..."
rg -g '!{*.lock,*.min.*}' "interface.*Form" -A 10
# Search for form initialization methods
echo "Searching for form initialization methods..."
rg -g '!{*.lock,*.min.*}' "initForm" -A 5
Length of output: 69317
Script:
#!/bin/bash
# Search for form initialization in composables
echo "Checking form initialization composable..."
cat client/composables/forms/initForm.js
# Search for form data structure
echo "Searching for form data structure..."
rg -g '!{*.lock,*.min.*}' "interface FormData" -A 10
Length of output: 3830
api/app/Http/Requests/UserFormRequest.php (1)
32-32: Verify Form::FORMATS constant exists
The validation rule references Form::FORMATS which should contain the allowed format values ('regular', 'focused').
✅ Verification successful
Form::FORMATS constant is properly defined with expected values
The constant exists in api/app/Models/Forms/Form.php and contains the expected format values: 'regular' and 'focused'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Form::FORMATS constant exists and contains expected values
rg -A 2 "const FORMATS = \[" api/app/Models/Forms/Form.php
Length of output: 185
client/composables/forms/initForm.js (1)
13-13: LGTM! Default format initialization looks good
The default format is correctly set to "regular" in the form initialization.
client/components/open/forms/components/form-components/AddFormBlock.vue (1)
90-91: LGTM! Format check implementation looks good
The computed property correctly determines if the form is in focused mode.
client/components/open/forms/components/form-components/FormEditorPreview.vue (3)
92-106: LGTM! Clean implementation of centered layout
The conditional absolute positioning with flexbox centering is a good approach for the focused mode layout.
135-136: LGTM! Clean computed property implementation
The computed property follows Vue.js best practices and provides a clear way to check the form format.
61-62: Consider viewport height implications
Using 100vh for the cover picture in focused mode might cause issues:
- Mobile browsers with dynamic toolbars can cause unexpected scrolling
- Content might be cut off on smaller screens
Also applies to: 67-68
client/components/open/forms/components/form-components/FormCustomization.vue (2)
47-56: LGTM! Clear and user-friendly format selection
The format options are well-labeled and the implementation follows component conventions.
57-60: Verify modal behavior across different scenarios
The modal integration looks good, but we should verify:
- Modal state reset on component unmount
- Handling of modal dismissal
✅ Verification successful
Modal implementation is properly handled
The FormatChangeModal component has proper state management and cleanup:
- Modal visibility is controlled by the
showprop with proper two-way binding - Modal dismissal is handled through the
onCancelmethod which resets the form format and emits close event - No explicit unmount cleanup is needed as the component uses
<script setup>with no persistent state - Format change logic properly filters out incompatible blocks and updates the form state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check FormatChangeModal implementation
ast-grep --pattern 'class FormatChangeModal {
$$$
beforeUnmount() {
$$$
}
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Let's try a different approach to find the FormatChangeModal component
# First, let's locate the file
fd "FormatChangeModal" --type f
# Then examine its content
rg -A 5 "FormatChangeModal" --type vue
Length of output: 151
Script:
#!/bin/bash
# Now that we found the file, let's examine its content
cat client/components/open/editors/FormatChangeModal.vue
Length of output: 1787
api/app/Models/Forms/Form.php (2)
44-45: LGTM! Consistent constant definition
The FORMATS constant follows the established pattern and aligns with the frontend implementation.
59-59: Verify database migration and default values
The format field is properly added to fillable attributes. Ensure there's a corresponding migration with a default value.
✅ Verification successful
Migration exists with default value - format field is properly configured
The migration file 2024_12_04_163142_add_format_to_forms_table.php adds the format column with a default value of 'regular'. The factory also includes this field with the same default value. The database schema is properly configured for this fillable attribute.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for migration file adding the format column
fd "add_format_to_forms" --type php
Length of output: 206
Script:
#!/bin/bash
# Search for migration files containing 'format'
rg -l "format" "database/migrations"
# Search for schema definition of 'format' column
rg -A 5 "format" "database/migrations"
Length of output: 191
Script:
#!/bin/bash
# Find all migration files in the repository
fd "\.php$" --type f
# Search for any file containing 'format' column definition
rg -A 5 "format" --type php
Length of output: 68868
client/components/open/forms/OpenForm.vue (1)
278-278: LGTM! Null check prevents potential runtime errors.
The added null check for previousFields is a good defensive programming practice that prevents potential runtime errors.
| const isFocused = computed(() => form.value.format === 'focused') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for form.value.
The computed property could throw an error if form.value is null during initial load or after errors.
Consider this safer implementation:
-const isFocused = computed(() => form.value.format === 'focused')
+const isFocused = computed(() => form.value?.format === 'focused')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isFocused = computed(() => form.value.format === 'focused') | |
| const isFocused = computed(() => form.value?.format === 'focused') | |
| class="w-full overflow-hidden flex items-center justify-center" | ||
| :class="{'h-screen': isFocused, 'max-h-56': !isFocused}" |
There was a problem hiding this comment.
💡 Codebase verification
Add keyboard controls for exiting full-screen focused mode
The verification reveals that while there's a computed property isFocused controlling the full-screen behavior, there are no keyboard event handlers to manage this state. To ensure accessibility:
- Add keyboard handlers (e.g., Escape key) to exit the focused mode
- Consider adding focus trap management when in full-screen mode
- Ensure the form content remains keyboard-accessible when cover picture is full-screen
🔗 Analysis chain
LGTM! Consider adding keyboard navigation for full-screen mode.
The dynamic height adjustment for focused mode is well implemented. However, when the cover picture is full-screen (h-screen), ensure that keyboard users can easily navigate past it.
Let's check if there are any accessibility-related components or keyboard handlers:
Also applies to: 17-17
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for keyboard navigation or accessibility-related code
rg -i "key(down|up|press)" client/pages/forms/
rg -i "a11y|accessibility" client/pages/forms/
Length of output: 97
Script:
#!/bin/bash
# Let's search for keyboard event handlers and focus management in the component
ast-grep --pattern 'v-on:keydown="$_"'
ast-grep --pattern '@keydown="$_"'
ast-grep --pattern '@keyup="$_"'
ast-grep --pattern '@keypress="$_"'
# Also check for any focus-related methods or computed properties
rg -A 5 "isFocused" client/pages/forms/
Length of output: 1832
| if (this.form.use_captcha && import.meta.client) { | ||
| this.dataForm['h-captcha-response'] = document.getElementsByName('h-captcha-response')[0].value | ||
| this.$refs.hcaptcha.reset() | ||
| } |
There was a problem hiding this comment.
Ensure the hCaptcha element exists before accessing its value.
When accessing document.getElementsByName('h-captcha-response')[0].value, there's a risk of encountering a runtime error if the element doesn't exist. Please add a check to ensure the element is present before accessing its value.
Apply this diff to prevent potential errors:
if (this.form.use_captcha && import.meta.client) {
- this.dataForm['h-captcha-response'] = document.getElementsByName('h-captcha-response')[0].value
+ const captchaElement = document.getElementsByName('h-captcha-response')[0]
+ if (captchaElement) {
+ this.dataForm['h-captcha-response'] = captchaElement.value
+ this.$refs.hcaptcha.reset()
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.form.use_captcha && import.meta.client) { | |
| this.dataForm['h-captcha-response'] = document.getElementsByName('h-captcha-response')[0].value | |
| this.$refs.hcaptcha.reset() | |
| } | |
| if (this.form.use_captcha && import.meta.client) { | |
| const captchaElement = document.getElementsByName('h-captcha-response')[0] | |
| if (captchaElement) { | |
| this.dataForm['h-captcha-response'] = captchaElement.value | |
| this.$refs.hcaptcha.reset() | |
| } | |
| } |
| <script setup> | ||
| import { defineProps } from "vue" | ||
|
|
||
| const props = defineProps({ | ||
| show: { type: Boolean, required: true }, | ||
| form: { type: Object, required: true }, | ||
| }) | ||
| </script> |
There was a problem hiding this comment.
Avoid directly mutating the form prop.
Modifying the form prop directly within FormFormatModal.vue violates Vue's best practices. Props should be treated as read-only. Consider using a local copy of the form data or emitting an event to update the parent component.
Apply this diff to create a local form copy:
<script setup>
import { defineProps, ref, toRefs } from "vue"
const props = defineProps({
show: { type: Boolean, required: true },
- form: { type: Object, required: true },
+ form: { type: Object, required: true },
})
+const localForm = ref({ ...props.form })
</script>And update the FlatSelectInput to use localForm:
<FlatSelectInput
- :form="form"
+ :form="localForm"
name="format"
...
/>Then, when the user clicks "Ok", emit an event to update the parent:
<UButton
size="md"
block
- @click="$emit('close')"
+ @click="updateFormat"
>
Ok
</UButton>
<script setup>
+function updateFormat() {
+ emit('update:format', localForm.value.format)
+ emit('close')
+}
</script>Committable suggestion skipped: line range outside the PR's diff.
| }) | ||
|
|
||
| const emit = defineEmits(['close']) | ||
| const removeTypes = ['nf-page-break','nf-divider','nf-image','nf-code'] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move removeTypes to a constants file
The removeTypes array should be moved to a constants file for better maintainability and reusability.
Create a new constant in a shared location:
-const removeTypes = ['nf-page-break','nf-divider','nf-image','nf-code']
+import { REMOVABLE_BLOCK_TYPES } from '@/constants/forms'
+const removeTypes = REMOVABLE_BLOCK_TYPESCommittable suggestion skipped: line range outside the PR's diff.
|
|
||
| <!-- Focused Format Options --> | ||
| <div | ||
| v-if="form.format === 'focused'" | ||
| class="px-4" | ||
| > | ||
| <EditorSectionHeader | ||
| icon="i-heroicons-bars-3-bottom-left" | ||
| title="Focused Format Options" | ||
| /> | ||
|
|
||
| <text-input | ||
| name="focused_previous_button_text" | ||
| :form="field" | ||
| label="Previous Button Text" | ||
| /> | ||
| <text-input | ||
| name="focused_next_button_text" | ||
| :form="field" | ||
| label="Next Button Text" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and default values for button text fields.
The focused format button text fields lack validation and default values. Consider:
- Adding validation to ensure non-empty values
- Setting default values in the component's created/mounted lifecycle hooks
- Adding help text to guide users
Apply these changes:
<text-input
name="focused_previous_button_text"
:form="field"
label="Previous Button Text"
+ :required="true"
+ help="Text to display on the previous button in focused mode"
+ placeholder="Previous"
/>
<text-input
name="focused_next_button_text"
:form="field"
label="Next Button Text"
+ :required="true"
+ help="Text to display on the next button in focused mode"
+ placeholder="Next"
/>And in the script section:
mounted() {
this.setDefaultFieldValues()
+ if (this.form?.format === 'focused') {
+ this.field.focused_previous_button_text = this.field.focused_previous_button_text || 'Previous'
+ this.field.focused_next_button_text = this.field.focused_next_button_text || 'Next'
+ }
}Committable suggestion skipped: line range outside the PR's diff.
…management - Introduced a new 'creating' Boolean prop in OpenForm.vue and OpenFormFocused.vue to manage form submission state. - Updated the nextPage method to include 'creating' in the condition for advancing the form page index, ensuring proper navigation during form creation. - Adjusted OpenCompleteForm.vue to bind the 'creating' prop, enhancing the overall functionality and user experience of the form components.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
client/components/open/forms/components/form-components/FormCustomization.vue (3)
47-56: Add help text to explain format implications.Consider adding help text to explain how each format affects form behavior and user experience. This will help users make an informed decision when selecting a format.
<FlatSelectInput :form="form" name="format" :options="[ { name: 'Regular format', value: 'regular' }, { name: 'Focused format (1 question per page)', value: 'focused' }, ]" label="Form format" + help="Regular format shows all questions at once. Focused format improves concentration by showing one question at a time." @update:model-value="onChangeFormat" />
162-162: Consider updating help text based on format.The label changes based on format, but the help text remains static. Consider updating the help text to provide format-specific guidance.
<image-input name="cover_picture" :form="form" :label="form.format === 'focused' ? 'Background image' : 'Cover image'" - help="Not visible when form is embedded" + :help="form.format === 'focused' + ? 'Background image appears behind each question in focused mode. Not visible when form is embedded' + : 'Cover image appears at the top of the form. Not visible when form is embedded'" />
283-287: Add error handling and documentation.The method could benefit from parameter validation and JSDoc documentation.
+/** + * Handles format change events from the format selector + * @param {string} val - The new format value ('regular' | 'focused') + */ const onChangeFormat = (val) => { + if (typeof val !== 'string' || !['regular', 'focused'].includes(val)) { + console.error('Invalid format value:', val); + return; + } if (val === 'focused') { showFormatChangeModal.value = true; } }client/i18n/lang/de.json (1)
6-7: Consider using more common German UI terminologyWhile the translations are grammatically correct, consider using more standard German UI terminology:
"back": "Zurück", - "next": "Nächste", + "next": "Weiter","Weiter" is more commonly used in German user interfaces for the "next" action and would provide a more familiar experience for German users.
client/components/open/forms/OpenFormFocused.vue (5)
21-39: Consider extracting the progress bar into a separate component.The progress bar implementation is duplicated between iframe and non-iframe versions with only minor styling differences. This violates the DRY principle.
Consider creating a new component like this:
+ <!-- ProgressBar.vue --> + <template> + <div :class="containerClass"> + <div class="w-full h-2 bg-gray-200 dark:bg-gray-600 relative overflow-hidden"> + <div + class="h-full transition-all duration-300" + :class="{ 'w-0': progress === 0 }" + :style="{ width: progress + '%', background: color }" + /> + </div> + </div> + </template>Then use it in both places:
- <div v-if="isIframe" class="mb-4 p-2"> - <div class="w-full h-2 bg-gray-200 dark:bg-gray-600 relative border rounded-full overflow-hidden"> - <!-- ... --> - </div> - </div> - <div v-else class="fixed top-0 left-0 right-0 z-50"> - <div class="w-full h-[0.2rem] bg-gray-200 dark:bg-gray-600 relative overflow-hidden"> - <!-- ... --> - </div> - </div> + <progress-bar + :progress="formProgress" + :color="form.color" + :container-class="isIframe ? 'mb-4 p-2' : 'fixed top-0 left-0 right-0 z-50'" + />
136-144: Add error handling for theme injection.The theme prop's default value implementation could fail if the component is used outside of a provide context. Consider adding proper error handling.
theme: { type: Object, default: () => { - const theme = inject("theme", null) - if (theme) { - return theme.value + try { + const theme = inject("theme", null) + return theme?.value ?? CachedDefaultTheme.getInstance() + } catch (error) { + console.warn('Theme injection failed, falling back to default theme:', error) + return CachedDefaultTheme.getInstance() } - return CachedDefaultTheme.getInstance() } }
206-214: Optimize formProgress computation.The current implementation iterates through fields twice. This can be optimized to a single pass.
formProgress() { - const requiredFields = this.fields.filter(field => field.required) - if (requiredFields.length === 0) { - return 100 - } - const completedFields = requiredFields.filter(field => ![null, undefined, ''].includes(this.dataFormValue[field.id])) - const progress = (completedFields.length / requiredFields.length) * 100 - return Math.round(progress) + let total = 0, completed = 0 + for (const field of this.fields) { + if (field.required) { + total++ + if (![null, undefined, ''].includes(this.dataFormValue[field.id])) { + completed++ + } + } + } + return total === 0 ? 100 : Math.round((completed / total) * 100) }
235-252: Optimize dataFormValue computation.The current implementation has nested loops for handling selection fields. Consider using a Map for O(1) lookups.
dataFormValue() { const data = this.dataForm.data() const selectionFields = this.fields.filter((field) => { return ['select', 'multi_select'].includes(field.type) }) selectionFields.forEach((field) => { if (data[field.id] !== undefined && data[field.id] !== null && Array.isArray(data[field.id])) { + const optionsMap = new Map(field[field.type].options.map(op => [op.id, op.name])) data[field.id] = data[field.id].map(option_nfid => { - const tmpop = field[field.type].options.find((op) => { - return (op.id === option_nfid) - }) - return (tmpop) ? tmpop.name : option_nfid + return optionsMap.get(option_nfid) ?? option_nfid }) } }) return data }
520-529: Enhance error handling in setMinHeight.The current implementation only logs errors to console without any recovery strategy.
setMinHeight(minHeight) { if (!this.isIframe) return this.minHeight = minHeight try { window.parentIFrame.size() } catch (e) { - console.error(e) + console.warn('Failed to resize iframe:', e) + // Fallback to default sizing mechanism + this.$nextTick(() => { + const element = this.$el + if (element) { + element.style.height = `${minHeight}px` + } + }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
api/app/Http/Requests/UserFormRequest.php(1 hunks)api/app/Models/Forms/Form.php(2 hunks)client/components/open/forms/OpenCompleteForm.vue(4 hunks)client/components/open/forms/OpenForm.vue(3 hunks)client/components/open/forms/OpenFormFocused.vue(1 hunks)client/components/open/forms/components/form-components/FormCustomization.vue(5 hunks)client/i18n/lang/ar.json(1 hunks)client/i18n/lang/bn.json(1 hunks)client/i18n/lang/de.json(1 hunks)client/i18n/lang/en.json(1 hunks)client/i18n/lang/es.json(1 hunks)client/i18n/lang/fr.json(1 hunks)client/i18n/lang/hi.json(1 hunks)client/i18n/lang/ja.json(1 hunks)client/i18n/lang/jv.json(1 hunks)client/i18n/lang/ko.json(1 hunks)client/i18n/lang/mr.json(1 hunks)client/i18n/lang/pa.json(1 hunks)client/i18n/lang/pt.json(1 hunks)client/i18n/lang/ru.json(1 hunks)client/i18n/lang/ta.json(1 hunks)client/i18n/lang/te.json(1 hunks)client/i18n/lang/tr.json(1 hunks)client/i18n/lang/ur.json(1 hunks)client/i18n/lang/vi.json(1 hunks)client/i18n/lang/zh.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- client/i18n/lang/zh.json
- client/i18n/lang/ar.json
- api/app/Http/Requests/UserFormRequest.php
- client/i18n/lang/en.json
- api/app/Models/Forms/Form.php
- client/i18n/lang/es.json
- client/i18n/lang/fr.json
- client/i18n/lang/pt.json
- client/i18n/lang/hi.json
- client/i18n/lang/ja.json
- client/components/open/forms/OpenForm.vue
- client/components/open/forms/OpenCompleteForm.vue
🔇 Additional comments (10)
client/components/open/forms/components/form-components/FormCustomization.vue (1)
229-229: LGTM!
The imports and reactive state management follow Vue's best practices.
Also applies to: 242-242
client/i18n/lang/ko.json (1)
6-7: LGTM! Korean translations are accurate
The translations for "back" (뒤로) and "next" (다음) are correct and follow standard Korean UI conventions.
client/i18n/lang/vi.json (1)
6-7: LGTM! Vietnamese translations are accurate
The translations for "back" (Trở lại) and "next" (Tiếp tục) are correct and follow standard Vietnamese UI conventions.
client/i18n/lang/bn.json (1)
6-7: LGTM! Bengali translations are accurate
The translations for "back" ("পেছনে") and "next" ("পরবর্তী") are correct and follow proper Bengali language conventions.
client/i18n/lang/tr.json (2)
6-7: LGTM! Turkish translations are accurate
The translations for "back" ("Geri") and "next" ("İleri") are correct and follow standard Turkish UI conventions.
6-7: Verify completeness of the Form focused mode implementation
While the navigation translations look good (except for the Urdu "next" translation), the PR title suggests a "Form focused mode" feature, but we only see language file changes. The AI summary also mentions format attributes and database schema changes.
Let's verify if all necessary files are included:
✅ Verification successful
Form focused mode implementation is complete and properly integrated
The implementation of the Form focused mode feature is complete and includes all necessary components:
- The format selection is implemented in
FormFormatModal.vueallowing users to choose between "Regular format" and "Focused format" - The focused mode implementation in
OpenFormFocused.vuehandles one-question-per-page display with proper navigation - The preview component in
FormEditorPreview.vuecorrectly renders the focused mode with proper styling - The translations for navigation buttons ("back" and "next") are properly integrated
The PR changes are consistent with the complete feature implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related changes in the codebase
# Look for files with "format" related changes
echo "Searching for format-related files..."
rg -l "format.*Form" --type php --type vue
# Look for database migration files
echo "Searching for migration files..."
fd -e php -p "database/migrations"
# Look for form-related component changes
echo "Searching for form components..."
rg -l "focused.*mode" --type vue
Length of output: 5422
Script:
#!/bin/bash
# Let's try a different approach to find Vue files and form-related changes
# Look for Vue files with form focus mode changes
echo "Searching for form focus mode in all files..."
rg -l "form.*focus"
# Look for recent migration about format
echo "Checking recent migration about format..."
cat "api/database/migrations/2024_12_04_163142_add_format_to_forms_table.php"
# Look for Vue files specifically
echo "Finding Vue files..."
fd -e vue
Length of output: 13594
Script:
#!/bin/bash
# Let's examine the focused form mode implementation and format changes
# Check the focused form implementation
echo "Checking OpenFormFocused.vue content..."
cat "client/components/open/forms/OpenFormFocused.vue"
# Check the format modal implementation
echo "Checking FormFormatModal.vue content..."
cat "client/components/pages/forms/create/FormFormatModal.vue"
# Check for format-related changes in the form editor
echo "Checking FormEditorPreview.vue content..."
cat "client/components/open/forms/components/form-components/FormEditorPreview.vue"
Length of output: 23477
client/i18n/lang/jv.json (1)
6-7: LGTM! Translations are accurate
The Indonesian translations for "back" ("Kembali") and "next" ("Lanjut") are correct and consistent with common UI terminology.
client/i18n/lang/ru.json (1)
6-7: LGTM! Translations are accurate and consistent
The Russian translations for "back" ("Назад") and "next" ("Вперед") are correct and align with common UI patterns. The translation for "back" is also consistent with the existing "goBack" translation in the file.
client/components/open/forms/OpenFormFocused.vue (2)
98-112: Ensure the hCaptcha element exists before accessing its value.
This is a duplicate of a previous review comment. The code still needs to check if the captcha element exists before accessing its value to prevent runtime errors.
534-538: LGTM!
The styling implementation is clean and follows Tailwind best practices.
| <FormatChangeModal | ||
| :show="showFormatChangeModal" | ||
| @close="showFormatChangeModal = false" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset format selection when modal is closed.
When the format change modal is closed (cancelled), the format selection should revert to 'regular'. This ensures the UI stays in sync with the actual format.
<FormatChangeModal
:show="showFormatChangeModal"
- @close="showFormatChangeModal = false"
+ @close="() => {
+ showFormatChangeModal = false;
+ if (form.format === 'focused') {
+ form.format = 'regular';
+ }
+ }"
/>Committable suggestion skipped: line range outside the PR's diff.
| "back": "ਪੁਛਣਾ", | ||
| "next": "ਪੁਛਣਾ", |
There was a problem hiding this comment.
Fix incorrect Punjabi translations
The current translations have two issues:
- Both "back" and "next" are translated to the same word "ਪੁਛਣਾ" (puchna)
- "ਪੁਛਣਾ" means "to ask" which is incorrect for navigation buttons
Please update the translations to:
- "back": "ਪੁਛਣਾ",
- "next": "ਪੁਛਣਾ",
+ "back": "ਪਿੱਛੇ",
+ "next": "ਅੱਗੇ",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "back": "ਪੁਛਣਾ", | |
| "next": "ਪੁਛਣਾ", | |
| "back": "ਪਿੱਛੇ", | |
| "next": "ਅੱਗੇ", |
| "back": "پیچھے", | ||
| "next": "پیچھے", |
There was a problem hiding this comment.
Fix incorrect Urdu translation for "next"
The translation for "next" is incorrectly set to "پیچھے" (meaning "back"), which is the same as the "back" translation. This would confuse users as both navigation buttons would show the same text.
Apply this fix:
"back": "پیچھے",
- "next": "پیچھے",
+ "next": "آگے",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "back": "پیچھے", | |
| "next": "پیچھے", | |
| "back": "پیچھے", | |
| "next": "آگے", |
| "back": "पुढे", | ||
| "next": "पुढे", |
There was a problem hiding this comment.
Fix incorrect Marathi translation for "back"
The translation for "back" is incorrect. Currently, both "back" and "next" are translated to "पुढे" (forward/next), which would confuse Marathi users.
Apply this correction:
- "back": "पुढे",
+ "back": "मागे",
"next": "पुढे",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "back": "पुढे", | |
| "next": "पुढे", | |
| "back": "मागे", | |
| "next": "पुढे", |
| "back": "పిలుస్తా", | ||
| "next": "పిలుస్తా", |
There was a problem hiding this comment.
Incorrect Telugu translations for navigation buttons
The current translations are incorrect:
- Both "back" and "next" are translated to "పిలుస్తా" (pilustā) which means "I will call"
- Using the same text for different actions will confuse users
Please update the translations to:
- "back": "పిలుస్తా",
- "next": "పిలుస్తా",
+ "back": "వెనుకకు",
+ "next": "తదుపరి",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "back": "పిలుస్తా", | |
| "next": "పిలుస్తా", | |
| "back": "వెనుకకు", | |
| "next": "తదుపరి", |
| "back": "பின்னுக்கு", | ||
| "next": "பின்னுக்கு", |
There was a problem hiding this comment.
Incorrect Tamil translation for "next" button
While "பின்னுக்கு" is correct for "back", using it for "next" is incorrect and will confuse users.
Please update the translations to:
"back": "பின்னுக்கு",
- "next": "பின்னுக்கு",
+ "next": "அடுத்து",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "back": "பின்னுக்கு", | |
| "next": "பின்னுக்கு", | |
| "back": "பின்னுக்கு", | |
| "next": "அடுத்து", |
| handleValidationError(error) { | ||
| console.error(error) | ||
| if (error?.data?.message) { | ||
| useAlert().error(error.data.message) | ||
| } | ||
| this.dataForm.busy = false | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in handleValidationError.
The current implementation only logs errors to console and shows a generic message. Consider providing more detailed error feedback to users.
handleValidationError(error) {
- console.error(error)
+ const errorDetails = error?.data?.errors || {}
+ const fieldErrors = Object.entries(errorDetails)
+ .map(([field, messages]) => `${field}: ${messages.join(', ')}`)
+ .join('\n')
+
if (error?.data?.message) {
- useAlert().error(error.data.message)
+ useAlert().error({
+ title: error.data.message,
+ message: fieldErrors || 'Please check your input and try again.',
+ duration: 5000
+ })
}
this.dataForm.busy = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleValidationError(error) { | |
| console.error(error) | |
| if (error?.data?.message) { | |
| useAlert().error(error.data.message) | |
| } | |
| this.dataForm.busy = false | |
| }, | |
| handleValidationError(error) { | |
| const errorDetails = error?.data?.errors || {} | |
| const fieldErrors = Object.entries(errorDetails) | |
| .map(([field, messages]) => `${field}: ${messages.join(', ')}`) | |
| .join('\n') | |
| if (error?.data?.message) { | |
| useAlert().error({ | |
| title: error.data.message, | |
| message: fieldErrors || 'Please check your input and try again.', | |
| duration: 5000 | |
| }) | |
| } | |
| this.dataForm.busy = false | |
| }, |
Summary by CodeRabbit
Release Notes
New Features
formatfield for forms, allowing users to select between "Regular" and "Focused" formats.formatproperty.Bug Fixes
Localization
UI Enhancements