-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix(input validation): fields are accepted infinity digits, form added #2235
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe pull request updates the PictureOptions and PictureSection components by introducing a unified form submission handler, modified input attributes, and refined event handling. PictureOptions now wraps its content in a form, utilizes default values, and accepts a new prop for controlling a popover. PictureSection adds state management for the popover using an isOpen flag and a handleOpen function. Additionally, the ResumeStore is enhanced with a new setValues method that enables batch updates via lodash with a debounced state update trigger. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FO as PictureOptions Form
participant RS as ResumeStore
U->>FO: Submit form data
FO->>FO: Execute handleFormSubmit
FO->>RS: Invoke setValues(payload)
RS-->>FO: Update resume data
sequenceDiagram
participant U as User
participant PS as PictureSection
participant PO as Popover
U->>PS: Click to toggle popover
PS->>PS: Execute handleOpen (toggle isOpen)
PS->>PO: Update popover "open" state
PO-->>PS: Render updated popover view
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/client/src/pages/builder/sidebars/left/sections/picture/options.tsx (1)
33-33
: Adjusted circle border radius from 9999 to 9998The border radius for circle was changed from 9999 to 9998. While this works, it would be helpful to document why this specific value was chosen.
Consider adding a comment explaining the reason for using 9998 instead of 9999 or another round number.
Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/client/src/pages/builder/sidebars/left/sections/picture/options.tsx
(5 hunks)apps/client/src/pages/builder/sidebars/left/sections/picture/section.tsx
(5 hunks)apps/client/src/stores/resume.ts
(2 hunks)
🔇 Additional comments (18)
apps/client/src/stores/resume.ts (2)
22-22
: Implement batch state updates with the new setValues methodThe new
setValues
method is a good addition that allows for efficiently updating multiple fields in one operation, reducing unnecessary re-renders.
44-52
: Good implementation of batch update functionalityThe implementation of
setValues
correctly handles the batch updates and properly triggers a debounced state update after applying all changes.apps/client/src/pages/builder/sidebars/left/sections/picture/section.tsx (5)
15-15
: Added useState import for popover state managementThe import has been updated to include useState, which is correctly used to manage the popover state.
24-24
: Added state management for popover visibilityThe isOpen state variable is properly initialized and used to control the popover visibility.
51-53
: Added handler to toggle popover visibilityThe handleOpen function correctly toggles the isOpen state, providing a clean way to control the popover.
88-88
: Popover component now uses controlled stateThe Popover component is now properly controlled by the isOpen state variable.
100-100
: PictureOptions now receives handleOpen propThe PictureOptions component now correctly receives the handleOpen function as a prop, allowing it to close the popover after form submission.
apps/client/src/pages/builder/sidebars/left/sections/picture/options.tsx (11)
2-11
: Updated imports to support form functionalityThe imports have been properly updated to include the necessary components and types for form handling.
42-46
: Added proper type definitionsThe types have been properly defined to make the component more type-safe, including the new PictureOptionsProps type.
48-51
: Updated component to accept handleOpen prop and use setValuesThe component now correctly accepts the handleOpen prop and uses the setValues method from the ResumeStore.
63-71
: Simplified event handlers for better readabilityThe event handlers have been simplified and now use more specific types, improving code clarity.
73-92
: Added form submission handler with batch updatesThe new handleFormSubmit function effectively processes form data and uses the setValues method to update multiple state values in a single operation. It also correctly calls handleOpen to close the popover after submission.
95-95
: Wrapped component in form element with proper structureThe component is now properly wrapped in a form element with an onSubmit handler and a submit button.
However, the Button should explicitly have type="submit" to clarify its purpose.
- <Button>{t`Save`}</Button> + <Button type="submit">{t`Save`}</Button>Also applies to: 235-236
99-107
: Updated Input props for size fieldThe Input component now uses defaultValue instead of value, which is appropriate for uncontrolled forms. The name attribute has been added to support form data collection.
However, consider using a more type-safe approach for the name attribute:
- name={"size"} + name="size"
120-135
: Added type="button" to ToggleGroupItemsThe ToggleGroupItems now correctly have type="button" to prevent them from submitting the form when clicked. This is a good accessibility improvement.
139-148
: Updated Input props for aspectRatio fieldThe Input component now uses defaultValue and includes a name attribute for form submission. The key attribute ensures the component re-renders when the value changes.
Similar to the size field, consider using a string literal instead of an object:
- name={"aspectRatio"} + name="aspectRatio"
162-177
: Added type="button" to border radius ToggleGroupItemsThe ToggleGroupItems for border radius now have type="button", which is consistent with the aspect ratio toggle group items.
181-190
: Updated Input props for borderRadius fieldThe Input component for border radius has been updated with similar improvements as the other fields.
Again, consider using a string literal for the name attribute:
- name={"borderRadius"} + name="borderRadius"
Fix for this issue: #2234 |
PR Description
Making input fields validation work as supposed previously.
Issue