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

Wrap fields with context to expose a useField hook #133

Merged
merged 5 commits into from
Dec 16, 2022

Conversation

bvangraafeiland
Copy link
Contributor

This hook can be used in any of the custom components, allowing those to change styling/behavior etc depending on the field's state. This is mostly useful for things like the required/error/dirty indicators (e.g. https://remix-forms.seasoned.cc/examples/render-field/required-indicator) which currently require the use of the renderField prop. See also #124.

Using the prop makes things quite verbose because the structure of each field will have to be replicated, even if just a single component needs to be customized. Since the default structure is not the same for all field types, customizing components in this way requires some gnarly copy and paste from the package's source code (see https://remix-forms.seasoned.cc/examples/render-field/inline-checkboxes).

For now, the PR only contains the code required to make useField work. If the maintainers agree with this approach, I'll add some examples to the docs as well (perhaps even replacing the current renderField section?), and tests where applicable.

@netlify
Copy link

netlify bot commented Dec 13, 2022

Deploy Preview for remix-forms ready!

Name Link
🔨 Latest commit 61d4c27
🔍 Latest deploy log https://app.netlify.com/sites/remix-forms/deploys/639cae333e61ba0008f91374
😎 Deploy Preview https://deploy-preview-133--remix-forms.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bvangraafeiland
Copy link
Contributor Author

Not sure why checks are failing - the build succeeds when I run it locally. If there's anything I can do about it, please let me know (I can't see the Netlify logs).

@gustavoguichard
Copy link
Contributor

Thank you for the PR @bvangraafeiland , will check the logs ASAP

@diogob
Copy link
Contributor

diogob commented Dec 14, 2022

@bvangraafeiland thanks again for the PR, I want to investigate an alternative implementation, thanks for the patience.

Copy link
Contributor

@diogob diogob left a comment

Choose a reason for hiding this comment

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

Please give me a day or two to play around with the implementation and then I'll add a proper review.

Copy link
Contributor

@diogob diogob left a comment

Choose a reason for hiding this comment

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

Only a few things before we can merge this one:

  • An example in apps/web/app/routes/examples/forms to show the use case.
  • An E2E test in apps/web/tests/examples/forms using the example and ensuring we can indeed render custom components that use this hook.
  • A rebase against the latest main branch.

@@ -279,6 +291,21 @@ function createField<Schema extends SomeZodObject>({
ref,
) => {
const value = fieldType === 'date' ? parseDate(rawValue) : rawValue
const field = {
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be great to reuse this constant to pass the childrenFn parameters (line 320) for the sake of readability, it makes it clearer that we are talking about the same field object.

Partial<Omit<Field<never>, 'name'>> | undefined
>(undefined)

export function useField() {
Copy link
Contributor

@diogob diogob Dec 15, 2022

Choose a reason for hiding this comment

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

what do yo think of the name userFieldContext ? It seems to make it clearer that the hook is returning a context and creates a neat parallel with the useFormContext provided by react-hook-form.
@danielweinmann thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like useFormContext and I think the only reason they named it like that is because useForm was already taken 😂

@bvangraafeiland
Copy link
Contributor Author

bvangraafeiland commented Dec 16, 2022

I've applied the feedback:

  • Added an example
  • Added an E2E test
  • Reused the field const in the childrenFn
  • Rebased the branch against main

However, the test is failing. This happens because the firstName field stays dirty after clearing it. This happens because the form is not rerendered, so the dirty prop is not updated. I'm not sure why this happens, but it doesn't have anything to do with the hook as far as I can tell (the same thing happens if you add the children function to Field). It only happens for fields that are not required and have no default value; adding either of those will cause the rerender to happen normally.

Before I spend too much time figuring this out, I thought maybe one of you immediately knows why this is? If not, maybe we can update the test for now (adding a default value for first name), and then add a new issue along with a PR providing the failing test case.

update: Actually it seems the dirty prop doesn't work at all when using a field with children - it's always undefined, because it's not passed to the childrenFn, see https://github.com/SeasonedSoftware/remix-forms/blob/main/packages/remix-forms/src/createField.tsx#L320-L348

@diogob
Copy link
Contributor

diogob commented Dec 16, 2022

@bvangraafeiland it's looking good. Good catch with the dirty missing in the childrenFn.

What do you think of simplifying your example and test cases? We just need to show useField being used once, so keeping only the custom Input would make the example easier to read for newcomers. Moreover, we can focus on the error indicator for now, this will make the tests green and I'll open another issue to fix the dirty indicator.

@bvangraafeiland
Copy link
Contributor Author

I've simplified the example and test, the test now also passes. The branch is no longer rebased to main but I believe you can do that when merging as well?

Copy link
Contributor

@diogob diogob left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gustavoguichard gustavoguichard left a comment

Choose a reason for hiding this comment

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

Great contribution @bvangraafeiland !

@diogob diogob merged commit 3b02f53 into seasonedcc:main Dec 16, 2022
@bvangraafeiland bvangraafeiland deleted the use-field-hook branch December 17, 2022 11:03
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.

4 participants