Skip to content
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

Checkbox, Radio, and Select dropdown field input on form builder #703

Merged
merged 17 commits into from
Oct 10, 2024

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Oct 8, 2024

Issue(s) Resolved

Closes #614
Closes #612
Closes #613

High-level Explanation of PR

Adds the checkbox group, radio group, and select dropdown field as options for StringArray and NumericArray fields in the form builder.

Test Plan

  1. Add a StringArray field
  2. Add a NumericArray field
  3. Add these to your form. You should see the option to add them either as a checkbox group, a radio group, or a select dropdown. You can click between these and the form should change a bit between components. Design
  4. You should be able to fill things out and then check in your db that the config obj of your form element saved properly.

Screenshots (if applicable)

Screen.Recording.2024-10-09.at.2.16.20.PM.mov

Notes

I had to redo the RadioGroupCard that holds the InputComponent options since that renders as a button. This causes a hydration error since html doesn't allow buttons inside of buttons—in this case, we want to render our checkbox group/radio group/select dropdown inside the RadioGroupCard, and those components all render as buttons with our shadcn implementation. So I changed that bit of code to use an old school <input type="radio"/> instead to avoid rendering buttons in buttons.

This PR will conflict with #704 but #704 should make things better here!

@allisonking allisonking force-pushed the aking/614/checkbox-field-input branch from 90ac700 to fdd53bf Compare October 9, 2024 18:02
Comment on lines +223 to +224
// Some `config` schemas have extra values which persist if we don't Clean first
const cleanedConfig = Value.Clean(componentConfigSchemas[values.component], values.config);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for example, the RadioGroup has a field includeOther which the SelectDropdown does not have. without this line, if we had saved a component as a RadioGroup, then switched it to a SelectDropdown and saved that, the includeOther would still persist in config, when really we'd like config to look like what we expect of a SelectDropdown config

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I hadn't even considered that config values would persist when the component changes, although it makes sense! And I think it's a nice feature now that you've added this

@allisonking allisonking marked this pull request as ready for review October 9, 2024 18:21
Copy link
Member

@kalilsn kalilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work overall! I have a few UX notes/nits, but they don't have to be blockers if you think they'd be better handled in the future.

Comment on lines +223 to +224
// Some `config` schemas have extra values which persist if we don't Clean first
const cleanedConfig = Value.Clean(componentConfigSchemas[values.component], values.config);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I hadn't even considered that config values would persist when the component changes, although it makes sense! And I think it's a nice feature now that you've added this

<div key={c}>
{/* We use regular input instead of a RadioGroup here because the RadioGroup
renders buttons, and we cannot render buttons inside of buttons. Some of the demo components
need to render buttons. */}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! this seems to work about as well as the cards did although there's a small regression where clicking inside the demo component's inputs doesn't bubble up to select a different radio input.

i think it might be ok to merge as is but if so we should add a ticket to improve these a bit by making it harder to interact with the demo component inputs through some or all of:

  • removing them from the tab sequencing
  • maybe disabling the inputs when they're not in a currently selected component?
  • bubbling click events up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I learned about inert today which seems like a pretty good use case for this—we don't really want those demo components interactive, but they can be if they're currently selected, so that the user can get a feel for them I guess?

here's the behavior now. so they are only part of the tab order if the item is selected. maybe they shouldn't be part of tab order at all? 🤔 but at least clicking on a radio button selects the component selector, if it wasn't selected already, and only if you click it again can you really click the radio

Screen.Recording.2024-10-10.at.12.40.01.PM.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice, I'm also just learning about inert for the first time! seems like that is exactly what we need!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking them out of the tab order entirely would be great but i think this is fine for now!

field.onChange(e);
}}
value={field.value ?? []}
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking the drag handle on an added option causes the form panel to close. I'm guessing it's a button without a type? It's not so bad since it's saving but probably worth fixing!

Screen.Recording.2024-10-09.at.5.07.42.PM.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, given how much this comes up, maybe we should default our Button component to type="button"? I doubt we're relying on that defaulting to submit anywhere

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favor of setting type="button" to be the default, type="submit" as the default (in general) was always confusing to me. I get that that was the only way you could use a button in ye olden days, but I think specifically saying "This button is used to submit a form" is obviously the case you would want to call out by setting the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh great catch! I've confirmed that changing the Button component's type to button fixes things, so I'll make that the default 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well i guess we were relying on that in some places. but i'm glad we're not now!

Copy link
Member

@kalilsn kalilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job!

@allisonking allisonking merged commit da94142 into main Oct 10, 2024
8 checks passed
@allisonking allisonking deleted the aking/614/checkbox-field-input branch October 10, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkbox Field Input Dropdown Field Input Radio Button Field Input
3 participants