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

feat(app): deprecate formik, migrate to react-hook-form #14424

Merged
merged 13 commits into from
Feb 28, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Feb 5, 2024

closes RAUT-960

Overview

Deprecate formik and update to react-hook-form. This migration has already been done in PD.

Test Plan

The following areas had components that were migrated, please check them to make sure they still work as expected:

  1. renaming robot on both app and ODD
  2. pipette settings slideout
  3. changing robot IP on the app from the general settings
  4. manually adding wifi name and the different options in the networking tab

Changelog

  • remove formik usage and replace with the equivalent react-hook-form components
  • fix tests

Review requests

see test plan

Risk assessment

low-ish, only a few components in the app were using formik

@jerader jerader added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Feb 5, 2024
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: Patch coverage is 70.81081% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 67.86%. Comparing base (63a20fc) to head (867e8ec).
Report is 68 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14424      +/-   ##
==========================================
+ Coverage   67.77%   67.86%   +0.09%     
==========================================
  Files        2518     2519       +1     
  Lines       71968    73051    +1083     
  Branches     9244     9722     +478     
==========================================
+ Hits        48774    49577     +803     
- Misses      20990    21201     +211     
- Partials     2204     2273      +69     
Flag Coverage Δ
app 64.79% <70.81%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...src/organisms/AppSettings/ManualIpHostnameForm.tsx 96.42% <95.00%> (+4.42%) ⬆️
...ings/ConnectNetwork/ConnectModal/SecurityField.tsx 23.07% <0.00%> (ø)
...edTab/AdvancedTabSlideouts/RenameRobotSlideout.tsx 92.45% <93.54%> (+11.05%) ⬆️
...tings/ConnectNetwork/ConnectModal/KeyFileField.tsx 25.00% <0.00%> (ø)
...Settings/ConnectNetwork/ConnectModal/TextField.tsx 14.28% <0.00%> (ø)
app/src/pages/NameRobot/index.tsx 82.60% <93.75%> (+9.72%) ⬆️
...Settings/ConnectNetwork/ConnectModal/FormModal.tsx 33.33% <16.66%> (-5.56%) ⬇️
...src/organisms/ConfigurePipette/ConfigFormGroup.tsx 50.00% <50.00%> (-2.00%) ⬇️
...ettings/ConnectNetwork/ConnectModal/form-fields.ts 84.41% <57.14%> (-12.73%) ⬇️
app/src/organisms/ConfigurePipette/ConfigForm.tsx 75.29% <82.22%> (+1.22%) ⬆️
... and 2 more

... and 52 files with indirect coverage changes

@jerader jerader marked this pull request as ready for review February 7, 2024 17:28
@jerader jerader requested review from a team as code owners February 7, 2024 17:28
@jerader jerader requested review from brenthagen and removed request for a team February 7, 2024 17:28
@jerader jerader removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Feb 7, 2024
@jerader jerader requested review from a team and koji February 7, 2024 17:29
Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

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

some initial comments testing the forms

@jerader jerader force-pushed the app_deprecate-formik branch from d66505c to 19d8e5b Compare February 8, 2024 20:37
@jerader jerader requested review from brenthagen and a team February 26, 2024 16:07
app/package.json Outdated Show resolved Hide resolved
@koji
Copy link
Contributor

koji commented Feb 26, 2024

tried this branch on a dev kit and rename worked as expected.
thank you for replacing!

>
<ConnectModalComponent {...props} />
</Formik>
<form onSubmit={handleSubmit(onSubmit)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

this form isn't submitting on click - tracked down the issue, the id (below in line 90) needs to be on this form element to associate the submit button in FormModal with this form.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you for thoroughly testing and catching this Brent!

Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

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

everything works, looks good!

@jerader jerader merged commit c2cce41 into edge Feb 28, 2024
22 checks passed
@jerader jerader deleted the app_deprecate-formik branch February 28, 2024 15:39
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.

3 participants