-
Notifications
You must be signed in to change notification settings - Fork 506
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
experimental/SelectPanel: Don't render form if nested inside form #4552
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this error is triggered when clicking the SelectPanel 👍
<form>
cannot appear as a descendant of<form>
.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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