experimental/SelectPanel: Don't render form if nested inside form #4552
Conversation
🦋 Changeset detectedLatest commit: bd12791 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
There was a problem hiding this comment.
Thanks for fixing this. As you mentioned, it might be better to not render a <form> element for SelectPanel at all, and that would fix this issue if someone had SelectPanel nested within a <form> other than FormControl. However, I'm not sure what the other implications of doing so are.
| <FormControl.Label>SelectPanel within FormControl</FormControl.Label> | ||
| <SelectPanel title="Choose a tag" selectionVariant="instant" onSubmit={onSubmit}> | ||
| <SelectPanel.Button leadingVisual={TagIcon}>{selectedTag || 'Choose a tag'}</SelectPanel.Button> | ||
| <form> |
There was a problem hiding this comment.
Now this error is triggered when clicking the SelectPanel 👍
<form>cannot appear as a descendant of<form>.
There was a problem hiding this comment.
Shouldn't this error have been showing without needing to wrap <form> around <FormControl>? Do we know why it was occurring in Dotcom and not Storybook and if there's a way to achieve parity between the two to avoid future problems?
There was a problem hiding this comment.
Shouldn't this error have been showing without needing to wrap
<form>around<FormControl>
Nope, because FormControl does not render a form, there was no outer/parent <form>
Do we know why it was occurring in Dotcom and not Storybook and if there's a way to achieve parity between the two to avoid future problems?
The dotcom version had an outer <form>, storybook didn't until now. FormControl should probably warn you if you are using it without a <form> 🤔
| @@ -306,8 +312,8 @@ const Panel: React.FC<SelectPanelProps> = ({ | |||
| }} | |||
| > | |||
| <Box | |||
| as="form" | |||
| method="dialog" | |||
| as={isInsideForm ? 'div' : 'form'} | |||
There was a problem hiding this comment.
Is it possible to write a test for this to ensure we're not getting the error again? Or is it not detectable through testing?
Note
If we want, we can wait until we figure out the semantics for https://github.com/github/primer/issues/3183 to find out if we want to render a
<form>element at allBug:
SelectPanel renders a
<form>element inside a<dialog>. We also do allow the use of SelectPanel inside with FormControl (which is expected to be inside a<form>already)This would create invalid DOM nesting of
<form>nested inside<form>. react-dom throws a validation warning on the consoleFix:
If SelectPanel is nested inside FormControl, it will now render a
<div>instead of<form>.Note: If someone is using SelectPanel in a custom form without FormControl, this PR would not be able to catch that scenario.
Rollout strategy
Testing & Reviewing
Merge checklist