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: legg til bedre støtte for react-hook-form i radioknappene #2443

Merged
merged 12 commits into from
Nov 9, 2021

Conversation

wkillerud
Copy link
Contributor

ISSUES CLOSED: #2435

☑️ Sjekkliste

  • Jeg har lest CONTRIBUTING og dokumentasjon det henvises til
  • Jeg har satt target branch til main, eller external-contributions dersom pull requesten kommer fra en fork
  • Jeg har kjørt yarn build og yarn ci:test og disse gir ingen feil
  • Jeg har lagt til tester som demonstrerer at feilen er rettet eller featuren fungerer
  • Jeg har skrevet relevant dokumentasjon

@wkillerud wkillerud added ✨ Forslag Forslag til nye funksjoner og endringer ✅ bugfix labels Nov 4, 2021
@wkillerud wkillerud requested a review from joms November 4, 2021 11:54
@wkillerud wkillerud self-assigned this Nov 4, 2021
@wkillerud wkillerud force-pushed the radio-buttons-react-hook-form branch from c73a3b2 to b0ce108 Compare November 5, 2021 09:36
@piofinn
Copy link
Contributor

piofinn commented Nov 5, 2021

Dette blir en morsom refaktoreringsjobb! 😅

@wkillerud
Copy link
Contributor Author

Dette blir en morsom refaktoreringsjobb! 😅

Er åpen for innspill her, men synes skjemakomponentene burde fungere godt med react-hook-form siden det tilnærmet er en de facto standard rundtomkring. Ser du noen umiddelbare ting som blir vondt å refactore?

@piofinn
Copy link
Contributor

piofinn commented Nov 5, 2021

Er åpen for innspill her

Det var ikke ment som kritikk, altså! 😅 Vi bør absolutt få denne til å funke med hook-form uten å måtte bruke <Controller>, og da er nok en composite component som dette den beste veien å gå 😊

joms
joms previously approved these changes Nov 8, 2021
Copy link
Contributor

@joms joms left a comment

Choose a reason for hiding this comment

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

Dette ser veldig likandes ut! Blir digg å kaste alle controllerene på skogen :D

HavardPede
HavardPede previously approved these changes Nov 8, 2021
Copy link

@HavardPede HavardPede left a comment

Choose a reason for hiding this comment

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

Jeg kjenner lite til react-hook-form. Er dette et bibliotek som er brukt av flere teams i Fremtind?

Ettersom det nå blir lagt opp en guide for bruk av react-hook-form i migration filen, burde det legges opp som en separat doc også? fks under "Kom igang"?

@wkillerud
Copy link
Contributor Author

wkillerud commented Nov 9, 2021

Jeg kjenner lite til react-hook-form. Er dette et bibliotek som er brukt av flere teams i Fremtind?

Det er brukt i ganske mange team ja, såpass at det bør være et mål for skjemakomponentene i Jøkul å fungere på en "komfortabel" (i mangel av et bedre ord her på morgenen) måte med det. Radioknapper er ikke de eneste som kan trenge litt kjærleik i så måte, men vi starter her 😄

Ettersom det nå blir lagt opp en guide for bruk av react-hook-form i migration filen, burde det legges opp som en separat doc også? fks under "Kom igang"?

Det er en god idé. Det har kanskje et hjem under https://jokul.fremtind.no/komigang/bygg 🤔

Edit: dokumentert her: https://github.com/fremtind/jokul/pull/2443/files#diff-8bcc495c98b292ec26d39e77e0969707ce91aa3905d1be3c072e219e45bd74f8

@wkillerud wkillerud dismissed stale reviews from HavardPede and joms via a5d19ff November 9, 2021 08:51
@wkillerud wkillerud force-pushed the radio-buttons-react-hook-form branch from b0ce108 to a5d19ff Compare November 9, 2021 08:51
wkillerud and others added 10 commits November 9, 2021 11:52
affects: @fremtind/jkl-field-group-react

Spre ...rest på fieldset også
affects: @fremtind/jkl-radio-button-react

BREAKING CHANGE:
Komponenten har fått et helt nytt API. Se migreringsdok.

ISSUES CLOSED: #2435
affects: @fremtind/jkl-feedback-react

BREAKING CHANGE:
Avhenger av jkl-radio-button med breaking change
…test

affects: @fremtind/jkl-alert-message-react
Comment on lines +26 to +27
security-events: write
pull-requests: read
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wkillerud
Copy link
Contributor Author

Jeg forsår ikke helt hvorfor snapshots ikke starter et nytt bygg nå som trigger er push. Eneste forskjell jeg kan se fra den gamle Cypress-workflowen er at vi pleide gjøre en checkout med token: ${{ secrets.BOT_PUBLISH_TOKEN }}. Husker du om det var noen grunn til det @piofinn?

@piofinn
Copy link
Contributor

piofinn commented Nov 9, 2021

Jeg forsår ikke helt hvorfor snapshots ikke starter et nytt bygg nå som trigger er push. Eneste forskjell jeg kan se fra den gamle Cypress-workflowen er at vi pleide gjøre en checkout med token: ${{ secrets.BOT_PUBLISH_TOKEN }}. Husker du om det var noen grunn til det @piofinn?

Det var nok en grunn til det, men hva den var husker jeg ikke 😅 Det er nok mulig at commits lagt inn via actions ikke teller som push. Mener kanskje jeg har lest noe om det inne i jungelen som er Actions-dokumentasjonen en gang 🤔

@wkillerud wkillerud merged commit cdc4e5a into main Nov 9, 2021
@wkillerud wkillerud deleted the radio-buttons-react-hook-form branch November 9, 2021 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Forslag Forslag til nye funksjoner og endringer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feil: ref på RadioButtons fungerer ikke som forventet
5 participants