form control support for select #1135
Conversation
WalkthroughThe changes introduce an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant SelectPrimitiveRoot
participant NativeSelect
User->>Form: Submit form
Form->>SelectPrimitiveRoot: Collect selected value
SelectPrimitiveRoot->>NativeSelect: Set value (hidden)
Form->>Form: Gather all form data (including hidden select)
Form->>User: Display submitted data
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (3)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
61-69: Optimize the hidden select rendering and handle edge cases.The hidden select implementation is functionally correct, but there are a few improvements to consider:
- Conditional rendering: The select should only render when
nameis provided- Empty value handling: Consider what happens when
selectedValueis empty or undefined- Option text: Using
selectedValueas option text may not be user-friendly if it's a technical identifierApply this diff to address these concerns:
{children} - {/* Add hidden native select for form control */} - <select - name={name} - value={selectedValue} - hidden - aria-hidden="true" - tabIndex={-1} - > - <option value={selectedValue}>{selectedValue}</option> - </select> + {/* Add hidden native select for form control */} + {name && ( + <select + name={name} + value={selectedValue || ''} + hidden + aria-hidden="true" + tabIndex={-1} + readOnly + > + <option value={selectedValue || ''}>{selectedValue || ''}</option> + </select> + )}The changes include:
- Conditional rendering only when
nameis provided- Safe handling of empty/undefined
selectedValue- Added
readOnlyattribute to prevent direct interactionsrc/core/primitives/Select/stories/Select.stories.tsx (2)
90-94: Improve consistency between custom and native select.For better comparison and validation, consider adding a default value to the native select to match the potential behavior of the custom select.
Apply this diff to improve consistency:
<select name="nativeSelect"> + <option value="">Select an option</option> <option value="option1">Option 1</option> <option value="option2">Option 2</option> <option value="option3">Option 3</option> </select>
105-110: Enhance user experience with better feedback.Consider adding user feedback when no data has been submitted yet and improve the visual presentation.
Apply this diff to enhance the user experience:
{submittedData && ( <div className="mt-4 p-4 bg-gray-100 rounded"> <h3 className="font-bold mb-2">Submitted Form Data:</h3> <pre>{submittedData}</pre> </div> )} + {!submittedData && ( + <div className="mt-4 p-4 bg-gray-50 rounded"> + <p className="text-gray-600">Submit the form to see the captured data</p> + </div> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(2 hunks)src/core/primitives/Select/stories/Select.stories.tsx(1 hunks)
🔇 Additional comments (4)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (2)
15-15: Good addition of the optional name prop.The addition of the optional
nameprop aligns well with the PR objectives to enable form control support.
21-21: Function signature correctly updated.The destructuring properly includes the new
nameparameter.src/core/primitives/Select/stories/Select.stories.tsx (2)
62-74: Well-structured form handling logic.The form submission handler correctly prevents default behavior, extracts form data, and updates state. The implementation demonstrates proper React form handling patterns.
79-88: Good demonstration of the custom select with form integration.The custom select properly uses the
nameprop to enable form submission, which directly demonstrates the PR's objective.
| <Primitive.div {...props} className={className}> | ||
| {children} | ||
| {/* Add hidden native select for form control */} | ||
| <select |
There was a problem hiding this comment.
this doesnt conditionally render based on if form is present right? it will always render, either form or not
There was a problem hiding this comment.
Solved by using a hidden select tag inside the root of select component. Addition - a hook to conditional render the select tag if a form element is in the parent dom is in progress.
it is in progress
|
|
||
| let current: HTMLElement | null = element; | ||
| while (current) { | ||
| if (current.tagName === 'FORM') return true; |
There was a problem hiding this comment.
how do these work? do we also need to check for lowercased form?
There was a problem hiding this comment.
The tests pretty much describe it all
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/hooks/useIsInsideFrom/index.ts (1)
19-19: The uppercase tagName check is correct.Addressing the past review comment: No need to check for lowercase
form. In HTML documents, thetagNameproperty always returns uppercase tag names, so checking for'FORM'is the correct approach.
🧹 Nitpick comments (1)
src/core/hooks/useIsInsideFrom/index.ts (1)
1-33: Fix the folder name typo.The folder name contains a typo:
useIsInsideFromshould beuseIsInsideFormto match the hook name and functionality.-src/core/hooks/useIsInsideFrom/index.ts +src/core/hooks/useIsInsideForm/index.ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/hooks/useIsInsideFrom/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/core/hooks/useIsInsideFrom/index.ts (1)
8-30: LGTM! Solid hook implementation.The hook implementation is well-structured with proper:
- Edge case handling for null elements
- Correct DOM traversal logic
- Appropriate React hook patterns with useState and useEffect
- Clear JSDoc documentation
The logic correctly identifies form ancestry by traversing up the DOM tree.
kotAPI
added
the
automerge
2 participants
#1128
Solved by using a hidden select tag inside the root of select component.
Addition - a hook to conditional render the select tag if a form element is in the parent dom is in progress.
Summary by CodeRabbit