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

Support render functions (+ misc tweaks) #157

Closed
wants to merge 5 commits into from

Conversation

kdonovan
Copy link

Hey, thanks for building this to DRY out the pain of wiring up react-bootstrap components to formik! Very helpful, but in my use-case I needed to pass a function as children, so I recreated the logic formik uses to handle either nested components or a function.

Once I figured out how to get yarn link working to test locally I added a couple other changes as well; feel free to add or ignore as you see fit -- each is in a separate commit.

Commits:

  • Support function children
  • Add groupClassName prop (gives control over FormGroup's className prop - I used groupClassName="w-100" to make textarea full width)
  • Added a default export for Form
  • Added Submit - just a small wrapper to add a spinner to the submit button when the form is submitting
  • Added a tiny usage example to the README (I found the previous "supports Input" statements very confusing at first, this section clarifies that components should be called like Form.Input).

Copy link
Owner

@tgfischer tgfischer left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! Left some comments, and open to discussing the API changes :)


const Component = (props) => (
<FormikForm
initialValues={foo: "Bar"}
Copy link
Owner

Choose a reason for hiding this comment

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

This should be {{ foo: "Bar" }}


### Example Usage

```ruby
Copy link
Owner

Choose a reason for hiding this comment

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

jsx here, not ruby

@@ -21,6 +22,7 @@ export const Input: FC<FormInputFieldProps> = ({
label={label}
helpText={helpText}
error={error}
className={groupClassName}
Copy link
Owner

Choose a reason for hiding this comment

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

I see the benefit, but I'm a bit uncomfortable about this API. What if other sub elements need the className set, or some other prop? In this case it's not a big deal since there is only a Group and Input, but just thinking about other components, and keeping a consistent API.

Open to other ideas as well :)

<Spinner size="sm" animation="border" className="mr-1" />
) : null}
{props.children}
</Button>
Copy link
Owner

Choose a reason for hiding this comment

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

I'm open to adding a SubmitButton, but I'm not sure about the Spinner. That will differ based on given designs, so might be better to leave it out and let the consumer extend it if they need to.

export * from "./components/Form";

export default Form;
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't need to do this, you can import it with a named import

import { Form } from "react-bootstrap-formik";

// or

import { Form as FormikForm } from "react-bootstrap-formik";

@tgfischer
Copy link
Owner

tgfischer commented Apr 14, 2021

... in my use-case I needed to pass a function as children, so I recreated the logic formik uses to handle either nested components or a function.

Also, I think adding this is useful since formik has a similar API. To hold you over, are you able to use useFormikContext?

import { useFormikContext } from "formik";
import { Form } from "react-bootstrap-formik";

const SamplePage = () =>(
  <Form>
    <SampleForm />
  </Form>
);

const SampleForm = () => {
  const { ... } = useFormikContext();
  return (
    <>
      <Form.Input name="foo" />
    </>
  )
}

@kdonovan
Copy link
Author

Thanks for the review! Feel free to tweak & merge -- in the intervening months I've just inlined the parts of the code I needed in my own app, so I'm not using the library anymore myself.

@tgfischer tgfischer closed this Apr 21, 2021
@tgfischer
Copy link
Owner

Going to close this for now then. Apologies for the delay!

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.

2 participants