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

Form: use context to handle coercions #834

Merged
merged 7 commits into from
Jul 15, 2020

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Jul 13, 2020

This is an alternative solution to #826 to handle nested form fields.

I feel this is a more idiomatic React solution, using Context instead of recursive looping over child elements.

Fixes #825

@thedavidprice
Copy link
Contributor

@cannikin it would be great to get this into the next release given you approval.

Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

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

Looks good to me in theory! Has someone actually built a form with fields nested in components and made sure it works?

@Tobbe
Copy link
Member Author

Tobbe commented Jul 14, 2020

@cannikin

The included test has this, where a <TextField> is nested in a <div>

  const TestComponentWithWrappedFormElements = ({ onSubmit = () => {} }) => {
    return (
      <Form onSubmit={onSubmit}>
        <p>Some text</p>
        <div className="field">
          <TextField name="wrapped-ff" defaultValue="3.14" dataType="Float" />
        </div>
        <Submit>Save</Submit>
      </Form>
    )
  }

Did you have something else in mind?

@cannikin
Copy link
Member

Ahhh, I don't know if it makes any difference but I thought this was so that if you have entire components nested inside the form, and those nested components have form fields, everything still works. But as far as React is concerned maybe just wrapping one in a <div> is functionally no different.

@Tobbe
Copy link
Member Author

Tobbe commented Jul 14, 2020

@cannikin
I extended the test a bit, can you please have a look again?
Also, there's a TODO in the code regarding ie11 support. I'm using Object.fromEntries which isn't available in ie11. What browsers do we want to support?

@Tobbe
Copy link
Member Author

Tobbe commented Jul 15, 2020

@cannikin Anything else you need from me to be able to merge this?

@cannikin
Copy link
Member

Also, there's a TODO in the code regarding ie11 support

Yikes, who added that? I haven't cared about IE11 for a decade! I don't think that we ever compiled an official browser support declaration. But shouldn't Babel be able to handle that for us anyway? I remember seeing something in a Babel config for >2% usage? Does IE11 still get used by 2% of people?

Otherwise looks good to me!

@Tobbe
Copy link
Member Author

Tobbe commented Jul 15, 2020

@cannikin I added that TODO to not forget about it when I wrote the code since I didn't know what browsers you all wanted to support. I've since rewritten the code to support IE11 as well, and obviously removed the TODO. I looked in to polyfills, but they were pretty big, so didn't want to add that. Was easy enough to just rewrite :)

I haven't cared about IE11 for a decade!

Lucky you! We still have to care about it on a daily basis at work ☹️

@cannikin
Copy link
Member

@thedavidprice Already approved!

@thedavidprice
Copy link
Contributor

@cannikin ha! I mean, ok then I'll be the one to merge. But you're assuming I know what I'm doing. (Scampers off to push the big 🚨 button...) Done!

Thanks @Tobbe 🎉

@thedavidprice thedavidprice merged commit 9dfb6de into redwoodjs:main Jul 15, 2020
@thedavidprice thedavidprice added this to the next release milestone Jul 15, 2020
@Tobbe Tobbe deleted the tobbe-825-context branch July 16, 2020 09:08
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.

CoercedValues function in web/form breaks forms that include any direct child node without props
3 participants