Rating field and single submit #387
Conversation
WalkthroughThe recent update introduces significant enhancements to form handling and integrations. It includes new functionality for handling actions based on a 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
composer.jsonis excluded by:!**/*.jsoncomposer.lockis excluded by:!**/*.locksrc/Blocks/components/input/manifest.jsonis excluded by:!**/*.jsonsrc/Blocks/components/rating/manifest.jsonis excluded by:!**/*.json
Files selected for processing (9)
- src/Blocks/components/form/assets/form.js (1 hunks)
- src/Blocks/components/form/assets/state.js (1 hunks)
- src/Blocks/components/input/components/input-editor.js (1 hunks)
- src/Blocks/components/input/components/input-options.js (7 hunks)
- src/Blocks/components/input/input-admin.scss (1 hunks)
- src/Blocks/components/input/input-editor.scss (1 hunks)
- src/Blocks/components/input/input-style.scss (1 hunks)
- src/Blocks/components/input/input.php (2 hunks)
- src/Blocks/components/phone/components/phone-options.js (1 hunks)
Files skipped from review due to trivial changes (1)
- src/Blocks/components/input/input-admin.scss
Additional comments: 12
src/Blocks/components/input/input-style.scss (1)
- 4-18: The styling for the 'range' input type looks good, ensuring a custom appearance for the slider thumb. It's important to verify cross-browser compatibility, especially for the
appearanceproperty and the::-webkit-slider-thumband::-moz-range-thumbpseudo-elements.src/Blocks/components/input/input-editor.scss (1)
- 6-7: The use of
!importantto remove padding for 'range' input types is noted. While the use of!importantis generally discouraged due to its impact on CSS specificity and maintenance, its application here seems necessary to ensure style overrides. Please ensure this is the only viable solution and consider documenting the reason for its use.src/Blocks/components/input/components/input-editor.js (1)
- 26-57: The implementation of the 'range' input type, including handling of
inputMin,inputMax, andinputStepattributes, is well done. The conditional rendering based oninputTypeis clear and effective. Please ensure that the use ofreadOnlyfor the 'range' input is intentional and consider adding comments to clarify the reasoning behind certain decisions, such as the default value handling and readOnly state.src/Blocks/components/input/input.php (1)
- 60-68: The implementation for supporting the 'range' input type, including handling of
min,max, andstepattributes, is correctly done. The use ofesc_attrfor attribute sanitization is good practice. Ensure that the default value handling and attribute sanitization meet all expected use cases.src/Blocks/components/phone/components/phone-options.js (1)
- 27-27: The addition of a comment to clarify the use of the
phoneIsNumberattribute is a good practice, enhancing code readability and maintainability. Well done on adding clear documentation.src/Blocks/components/input/components/input-options.js (1)
- 341-357: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [84-349]
The introduction of the
formatNumberfunction and its application toinputMin,inputMax, andinputStepattributes is a valuable enhancement for consistent number formatting. The updated conditions for setting attributes based on the 'number' and 'range' input types are well-implemented. Consider adding unit tests for theformatNumberfunction to ensure its correctness across a variety of inputs.src/Blocks/components/form/assets/state.js (5)
- 539-539: The use of optional chaining when accessing
element.closestingetFormElementByChildmethod is a good practice to prevent potential runtime errors whenelementis null or undefined. This change enhances the robustness of the code.- 542-542: Adding optional chaining in
getFormFieldElementByChildmethod forelement.closestcall is a prudent enhancement. It ensures that the code gracefully handles cases whereelementmight be null or undefined, avoiding possible runtime exceptions.- 545-545: The introduction of optional chaining in
getFormIdforelement.getAttributecall is a solid improvement. It safeguards against null or undefinedelement, preventing the code from throwing an error in such scenarios.- 548-548: Utilizing optional chaining in
getFormIdByElementwhen invokingthis.getFormElementByChild(element)?.getAttributeis an excellent way to ensure the code's resilience against null or undefined values. This change contributes to the overall stability of the application.- 551-551: Implementing optional chaining in
getFieldNameByElementfor thethis.getFormFieldElementByChild(element)?.getAttributecall is a wise decision. It effectively prevents errors ifelementis null or undefined, enhancing the code's error handling capabilities.src/Blocks/components/form/assets/form.js (1)
- 1506-1506: The use of optional chaining (
?.) when accessing thesubmitterproperty of theeventobject is a good practice to handle potentialnullorundefinedvalues safely. This change enhances the robustness of the event handling logic.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 2
CHANGELOG.md (2)
- 7-8: The addition of the new
rangefield for forms is well-documented. It's crucial to ensure that the documentation elsewhere in the project, such as the user guide or API documentation, is also updated to reflect this new feature.- 17-17: The removal of unnecessary options in the
ratingfield suggests a simplification of the user interface. It's important to communicate these changes to users, possibly through a user guide update or release notes, to ensure they are aware of the streamlined options.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Blocks/custom/form-selector/form-selector-editor.scss (2 hunks)
Files not summarized due to errors (1)
- src/Blocks/custom/form-selector/form-selector-editor.scss: Error: Message exceeds token limit
Additional comments: 3
src/Blocks/custom/form-selector/form-selector-editor.scss (3)
- 35-55: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-37]
The use of
!importantin CSS properties can make future maintenance and overrides more challenging due to increased specificity. While it's sometimes necessary, consider if there are alternative approaches to achieve the desired styling without!important, such as more specific selectors or reordering CSS rules.
- 35-55: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [38-71]
The addition of base64 encoded SVGs as background images is a neat way to embed images directly into the stylesheet, reducing HTTP requests. However, ensure that these images are optimized for size to avoid unnecessarily increasing the CSS file size. Additionally, using meaningful placeholders for form names enhances user experience by providing clear context.
- 35-55: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-73]
The overall structure of the file is well-organized, and the use of CSS custom properties for theme colors is a commendable practice. This approach enhances maintainability and consistency across the project. Keep up with these best practices for future development.
There was a problem hiding this comment.
Left some questions and suggestions.
The main question I have: why are you using shortcodes? They seem antiquated a bit.
Also, once you've addressed all the issues, you can either squash some of the commits, or squash all into one. There are a lot of 'update' commits that aren't saying much.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/Shortcode/ResultOutputItemPart.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Shortcode/ResultOutputItemPart.php
src/Blocks/custom/result-output-item/components/result-output-item-options.js
Outdated
Show resolved
Hide resolved
src/Blocks/custom/result-output/components/result-output-editor.js
Outdated
Show resolved
Hide resolved
src/Blocks/custom/result-output-item/components/result-output-item-editor.js
Outdated
Show resolved
Hide resolved
src/Blocks/custom/result-output-item/components/result-output-item-options.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Goran Alković <[email protected]>
Co-authored-by: Denis Žoljom <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
composer.jsonis excluded by:!**/*.json
Files selected for processing (1)
- CHANGELOG.md (2 hunks)
Additional comments: 2
CHANGELOG.md (2)
- 10-10: The term
singleSubmitis correctly used in the context of an attribute name. The suggestion to split it into "single Submit" is not applicable here.- 282-282: The version comparison links at the bottom of the file are a helpful addition for navigating changes between versions. No action is needed here, but it's good to acknowledge these thoughtful details.
Co-authored-by: Denis Žoljom <[email protected]> Co-authored-by: Ivan Ramljak <[email protected]> Co-authored-by: Goran Alković <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
composer.jsonis excluded by:!**/*.json
Files selected for processing (16)
- src/AdminMenus/FormAdminMenu.php (22 hunks)
- src/AdminMenus/FormListingAdminSubMenu.php (3 hunks)
- src/Blocks/components/form/assets/utils.js (2 hunks)
- src/Blocks/custom/result-output-item/components/result-output-item-editor.js (1 hunks)
- src/Blocks/custom/result-output-item/components/result-output-item-options.js (1 hunks)
- src/Blocks/custom/result-output-item/result-output-item-editor.scss (1 hunks)
- src/Blocks/custom/result-output/components/result-output-editor.js (1 hunks)
- src/Blocks/custom/result-output/components/result-output-options.js (1 hunks)
- src/Blocks/custom/result-output/result-output-editor.scss (1 hunks)
- src/CustomPostType/Result.php (1 hunks)
- src/Editor/Editor.php (2 hunks)
- src/General/SettingsGeneral.php (3 hunks)
- src/Integrations/Calculator/SettingsCalculator.php (1 hunks)
- src/Listing/FormsListing.php (3 hunks)
- src/Rest/Routes/AbstractFormSubmit.php (1 hunks)
- src/Rest/Routes/Settings/BulkRoute.php (6 hunks)
Files skipped from review as they are similar to previous changes (14)
- src/AdminMenus/FormListingAdminSubMenu.php
- src/Blocks/custom/result-output-item/components/result-output-item-editor.js
- src/Blocks/custom/result-output-item/components/result-output-item-options.js
- src/Blocks/custom/result-output-item/result-output-item-editor.scss
- src/Blocks/custom/result-output/components/result-output-editor.js
- src/Blocks/custom/result-output/components/result-output-options.js
- src/Blocks/custom/result-output/result-output-editor.scss
- src/CustomPostType/Result.php
- src/Editor/Editor.php
- src/General/SettingsGeneral.php
- src/Integrations/Calculator/SettingsCalculator.php
- src/Listing/FormsListing.php
- src/Rest/Routes/AbstractFormSubmit.php
- src/Rest/Routes/Settings/BulkRoute.php
Additional comments: 6
src/AdminMenus/FormAdminMenu.php (6)
- 13-14: Added imports for
ResultandFormsfromEightshiftForms\CustomPostTypeare correctly placed at the top of the file, following PSR-4 autoloading standards.- 193-193: The addition of handling for a new
parentparameter in theprocessAttributesmethod is a good practice for extending functionality. However, ensure that theparentparameter is properly sanitized and validated before use to prevent security vulnerabilities.- 218-280: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [197-257]
The updated cases in the
switchstatement to use constants fromUtilsConfigimprove code readability and maintainability by replacing magic strings with meaningful constant names.
- 225-255: The logic modification based on the
parentparameter in theprocessAttributesmethod is well-implemented. It allows for more granular control over the output based on the context, enhancing the flexibility of the method. Ensure that all possible values ofparentare accounted for and handled appropriately.- 292-294: The addition of new functionality for handling different actions based on the
parentparameter is a significant improvement. It's important to ensure that these actions are thoroughly tested, especially in edge cases where theparentparameter might not be set or could have unexpected values.- 538-586: Regarding the existing comments on lines 538-586 about the indentation in the provided suggestion, it's crucial to maintain consistent code formatting throughout the project. While the decision to leave the code as is for easier understanding is understandable, consider using a code formatter or linter to ensure consistency without sacrificing readability.
src/Blocks/custom/result-output-item/components/result-output-item-options.js
Outdated
Show resolved
Hide resolved
src/Blocks/custom/result-output/components/result-output-editor.js
Outdated
Show resolved
Hide resolved
src/Blocks/custom/result-output/components/result-output-options.js
Outdated
Show resolved
Hide resolved
…ns.js Co-authored-by: Ivan Ramljak <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/Blocks/custom/result-output/components/result-output-options.js (1 hunks)
- src/Editor/Editor.php (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/Blocks/custom/result-output/components/result-output-options.js
- src/Editor/Editor.php
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- src/Blocks/components/form/assets/utils.js (2 hunks)
- src/Blocks/components/input/components/input-editor.js (2 hunks)
- src/Blocks/custom/form-selector/components/form-selector-editor.js (1 hunks)
- src/Blocks/custom/forms/components/forms-editor.js (2 hunks)
- src/Blocks/custom/result-output-item/components/result-output-item-options.js (1 hunks)
- src/Blocks/custom/result-output/components/result-output-editor.js (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/Blocks/custom/forms/components/forms-editor.js
- src/Blocks/custom/result-output-item/components/result-output-item-options.js
- src/Blocks/custom/result-output/components/result-output-editor.js
Additional comments: 4
src/Blocks/components/input/components/input-editor.js (2)
- 26-29: The addition of
inputMin,inputMax, andinputStepattributes is a good enhancement for supporting range inputs. It's important to ensure that these attributes are properly validated and sanitized before use to prevent any potential security issues.- 33-36: As previously noted by a reviewer, using
classnamesfrom theclassnamespackage directly is not preferred due to tree-shaking concerns. It's good to see there's already an issue opened to address this in the future. Keep this in mind for upcoming changes.src/Blocks/custom/form-selector/components/form-selector-editor.js (2)
- 29-29: Updating the placeholder label to reference 'Eightshift Forms' correctly aligns with the project's branding. This change enhances clarity and consistency in the UI.
- 32-32: The updated prompt text asking about the type of the new form is clear and user-friendly. This change should help guide users more effectively when creating new forms.
Added
rangefield for the forms.singleSubmitattribute on all fields to allow only one submit per form to be used as calculation form.single submitoption to send data without submit button.Changed
Inputfield now output email and url as a correct type so it can be user on mobile devices.Fixed
Removed
ratingfield.Summary by CodeRabbit
InputEditorcomponent to support additional input properties for the 'range' input type.getEditorBackLinkmethod to handle multiple post types and URL redirection.SettingsGeneralclass for improved global settings management.