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

📝 Modal: Oppdatert JSDoc #2567

Merged
merged 4 commits into from
Dec 20, 2023
Merged

📝 Modal: Oppdatert JSDoc #2567

merged 4 commits into from
Dec 20, 2023

Conversation

HalvorHaugan
Copy link
Contributor

@HalvorHaugan HalvorHaugan commented Dec 18, 2023

Oppdatert JSDoc på Modal sin onBeforeClose og onCancel ihht. endringer i Chrome.

Valgte også å merke onBeforeClose som deprecated, da jeg tenker at man kanskje bør unngå å bruke den i utgangspunktet. Er imidlertid veldig usikker, da jeg ser at 2-3 repoer har brukt den til å spørre om bekreftelse før modalen lukkes. Kanskje de ønsker å beholde det selv om det ikke alltid vil funke.

#2555
https://bugs.chromium.org/p/chromium/issues/detail?id=1510389#c8
https://nav-it.slack.com/archives/C039XP6GF0S/p1702894915653149?thread_ts=1702662836.468729&cid=C039XP6GF0S

Copy link

changeset-bot bot commented Dec 18, 2023

🦋 Changeset detected

Latest commit: 5161b33

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-react Patch
@navikt/ds-css Patch
@navikt/ds-tokens Patch
@navikt/ds-tailwind Patch
@navikt/aksel-icons Patch
@navikt/aksel Patch
@navikt/aksel-stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 18, 2023

Storybook demo

7e6c4ce25 | 61 komponenter | 401 stories

@KenAJoh
Copy link
Collaborator

KenAJoh commented Dec 18, 2023

Er imidlertid veldig usikker, da jeg ser at 2-3 repoer har brukt den til å spørre om bekreftelse før modalen lukkes. Kanskje de ønsker å beholde det selv om det ikke alltid vil funke.

Vil dette mønsteret kunne erstatte onBeforeClose?:

onClose={() => {
  const confirmed = askUserToConfirmClose
  if(!confirmed){
    modalRef.current.showModal()
  }
}}

@HalvorHaugan
Copy link
Contributor Author

Det fungerer, men du får et "blink" når modalen åpnes igjen pga. animasjonene.

@KenAJoh
Copy link
Collaborator

KenAJoh commented Dec 18, 2023

Det fungerer, men du får et "blink" når modalen åpnes igjen pga. animasjonene.

Kunne wrappet showModal() i en https://react.dev/reference/react/useImperativeHandle for å støtte

modalRef.current.showModal({animation: false})

Men ikke så stor fan av å overskrive native-funksjonaliteten 🤔

@HalvorHaugan
Copy link
Contributor Author

Fjernet deprekeringen. Hvis folk først gjør dette, tror jeg det er like greit at de bruker onBeforeClose.
Har forslag til to justeringer i typene vi kan gjøre i v6 for å redusere misforståelser om onBeforeClose vs. onClose; kreve at man returnerer boolean i førstnevnte, og kreve at sistnevnte er definert hvis man har definert open.
Endringen i Chrome er dessuten veldig fersk, kanskje det kommer flere endringer, eller dukker opp bedre løsninger.

@KenAJoh
Copy link
Collaborator

KenAJoh commented Dec 19, 2023

Fjernet deprekeringen. Hvis folk først gjør dette, tror jeg det er like greit at de bruker onBeforeClose. Har forslag til to justeringer i typene vi kan gjøre i v6 for å redusere misforståelser om onBeforeClose vs. onClose; kreve at man returnerer boolean i førstnevnte, og kreve at sistnevnte er definert hvis man har definert open. Endringen i Chrome er dessuten veldig fersk, kanskje det kommer flere endringer, eller dukker opp bedre løsninger.

Du kan legge til en liten summary av dette her https://github.com/navikt/team-aksel/issues/327 så har vi det "logget" for v6 🚀 ✨

@HalvorHaugan HalvorHaugan merged commit 65f6074 into main Dec 20, 2023
2 checks passed
@HalvorHaugan HalvorHaugan deleted the upd-modal-jsdoc-cancel branch December 20, 2023 08:11
@github-actions github-actions bot mentioned this pull request Dec 20, 2023
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