Skip to content

Add react-hook-form to ImageForm #1310

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jonas-C
Copy link
Contributor

@Jonas-C Jonas-C commented Dec 28, 2021

Denne PR'en erstatter bruken av Formik i ImageForm med react-hook-form. Størrelsen på PR'en er ikke nødvendigvis representativ, da den inneholder en del filer som måtte "klones" for å slippe unna Formik.

Grunnen til at denne overgangen er interessant er at react-hook-form i mye større grad unngår rerenders når et form oppdateres. Vårt tidligere bruk av Formik førte til at hele siden ble rerendret hver gang det oppsto en endring i et felt. I React-hook-form skjer dette kun ved første tastetrykk i et felt. Forskjellene kan testes ut ved å åpne konsollen, navigere til en react-dev-tools-fane og sjekke "Highlight updates when components render".

React-hook-form har ikke setStatus, som vi har brukt til å eksplisitt fortelle Slate at en ny verdi skal tas i bruk når et form resettes til en tidligere versjon. Jeg endte opp med å lage en Provider som dispatcher events som kan lyttes på. Inntil videre opprettes en slik provider på toppen av hvert form i applikasjonen, slik at vi slipper å legge til flere globale providers. Inntil videre kan den legges inn v.h.a withFormEventsProvider, sånn at det er mest mulig "plug-and-play".

Det ser også ut som at react-hook-form er såpass god til diffing at vi ikke trenger å bruke isFormikFormDirty, men det kan hende at @haattis har andre meninger der. Krysser fingrene.

FormField er ment til å erstatte FormikField, bare at den ikke defaulter til Input dersom children ikke passeres. Man må alltid spesifisere hva som skal rendres, men dette kan fint endres. For å få til ordentlig typing av returverdiene til children-funksjonen måtte jeg ty til litt kreativ typing, da typene til react-hook-form ikke ville godta måten vi tar i bruk Descendant på.

Etter denne overgangen er flaskehalsen for et form nedtrekkspaneler. Når en accordion åpnes eller lukkes rerendres hele formet. Hele formet blir også rerendret hver gang et felt endrer gyldighet (valid->invalid / invalid->valid). Sistnevnte kan kanskje ordnes med memoization, det har jeg ikke fått sett på ennå. Førstnevnte vil være mer utfordrende, og kan heller tas på et annet tidspunkt. Da burde man også se på hvordan en AccordionSection blir rendret. Slik det er nå blir den gjenskapt hver gang en seksjon blir åpnet, som fører til unødvendige API-kall og renders.

…ase performance. Added a provider to allow slate to react to form events.
@cypress
Copy link

cypress bot commented Dec 28, 2021



Test summary

54 0 0 0


Run details

Project editorial-frontend
Status Passed
Commit 378c9cddf8 ℹ️
Started Dec 28, 2021 1:29 PM
Ended Dec 28, 2021 1:36 PM
Duration 07:11 💡
OS Linux Ubuntu - 20.04
Browser Electron 89

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@haattis haattis left a comment

Choose a reason for hiding this comment

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

Har testet og alt ser ut til å fungere fint! Har en liten kommentar bare :)

@@ -16,21 +16,16 @@ import { FieldHeader } from '@ndla/forms';
import { useTranslation } from 'react-i18next';
import Contributor from './Contributor';
import { ContributorType, ContributorFieldName } from './types';
import { ContributorType as ContributorTypeName } from '../../interfaces';
Copy link
Contributor

Choose a reason for hiding this comment

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

Foreslår å endre navn på typene for å unngå to exports med samme navn.

@Jonas-C
Copy link
Contributor Author

Jonas-C commented Apr 25, 2023

Har vi lyst til å gjøre noe med dette? Formik virker ikke altfor aktivt om dagen. RHF brukes også i ndla-frontend.

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