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(Forms): add support for defaultValue (and value) for fields used in Iterate.Array #3987

Merged
merged 3 commits into from
Oct 5, 2024

Conversation

tujoworker
Copy link
Member

Fixes #3882

Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eufemia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2024 8:53am

Copy link

codesandbox-ci bot commented Sep 24, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

@langz langz left a comment

Choose a reason for hiding this comment

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

🦭

@tujoworker tujoworker requested a review from andlbrei September 24, 2024 07:19
@tujoworker
Copy link
Member Author

@andlbrei Is it possible for you to test this feature manually? Would be really nice to get this confirmation before merge 🙏

Copy link
Contributor

@andlbrei andlbrei left a comment

Choose a reason for hiding this comment

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

As noted in DM with @tujoworker there is an issue when running the CSB used in the bug report. I tried adding a populated data object to the Form Handler, but it did not work then either.

TypeError
Cannot set properties of null (setting 'firstName')
Function.set
https://cdlmrl.csb.app/node_modules/json-pointer/index.js:101:18
    at eval (https://cdlmrl.csb.app/node_modules/
dnb/eufemia/extensions/forms/DataContext/Provider/Provider.js:406:28
    at eval (https://cdlmrl.csb.app/node_modules/
dnb/eufemia/extensions/forms/hooks/useFieldProps.js:937:99
commitHookEffectListMount
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:23150:26
invokeLayoutEffectMountInDEV
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:25120:13
invokeEffectsInDev
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:27351:11
commitDoubleInvokeEffectsInDEV
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:27327:5
flushPassiveEffectsImpl
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:27056:5
flushPassiveEffects
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:26984:14
performSyncWorkOnRoot
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:26076:3
flushSyncCallbacks
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:12042:22
commitRootImpl
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:26959:3
commitRoot
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:26682:5
finishConcurrentRender
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:25981:9
performConcurrentWorkOnRoot
https://cdlmrl.csb.app/node_modules/react-dom/cjs/react-dom.development.js:25809:7
workLoop
https://cdlmrl.csb.app/node_modules/scheduler/cjs/scheduler.development.js:266:34
flushWork
https://cdlmrl.csb.app/node_modules/scheduler/cjs/scheduler.development.js:239:14
MessagePort.performWorkUntilDeadline
https://cdlmrl.csb.app/node_modules/scheduler/cjs/scheduler.development.js:533:21

@tujoworker tujoworker force-pushed the fix/iterate-default-value branch from 423644f to 8b24e74 Compare September 24, 2024 17:24
@tujoworker
Copy link
Member Author

tujoworker commented Sep 24, 2024

Yes, I committed a solution for the problem + tests.

@andlbrei Can you verify that it works? 🙏

Here is the new CSB version: https://pkg.csb.dev/dnbexperience/eufemia/commit/8b24e742/@dnb/eufemia

Copy link
Contributor

@andlbrei andlbrei left a comment

Choose a reason for hiding this comment

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

Seems to work fine now 👍 Great work!

@andlbrei
Copy link
Contributor

andlbrei commented Sep 25, 2024

I tried the latest build in my project and found some different issues, I guess it's best to fix it in this MR. I could open a new issue if you'd like.

When adding multiple elements, it's not possible to remove elements. I guess it should be possible to have an empty list as well, if validation allows it, so I guess removing elements in general is not possible.

Also looking at the video it seems like defaultValue only works on the first item?

Screen.Recording.2024-09-25.at.09.46.14.mov

The other issue I am currently seeing if I can reproduce. It's the same error as before:

ERROR
Cannot set properties of null (setting 'phoneNumber')
TypeError: Cannot set properties of null (setting 'phoneNumber')
    at Function.set (webpack-internal:///../../node_modules/.pnpm/[email protected]/node_modules/json-pointer/index.js:85:18)
    at eval (webpack-internal:///../../node_modules/.pnpm/@pkg.csb.dev+dnbexperience+eufemia+commit+89ea339f+@dnb@eufemia_@[email protected]_@typ_u6l6ah63kdsofkxxrsacxiqmie/node_modules/@dnb/eufemia/extensions/forms/DataContext/Provider/Provider.js:490:65)
    at eval (webpack-internal:///../../node_modules/.pnpm/@pkg.csb.dev+dnbexperience+eufemia+commit+89ea339f+@dnb@eufemia_@[email protected]_@typ_u6l6ah63kdsofkxxrsacxiqmie/node_modules/@dnb/eufemia/extensions/forms/hooks/useFieldProps.js:1204:105)
    at eval (webpack-internal:///../../node_modules/.pnpm/@pkg.csb.dev+dnbexperience+eufemia+commit+89ea339f+@dnb@eufemia_@[email protected]_@typ_u6l6ah63kdsofkxxrsacxiqmie/node_modules/@dnb/eufemia/extensions/forms/hooks/useFieldProps.js:1258:13)
    at commitHookEffectListMount (webpack-internal:///../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:19590:42)
    at commitLayoutEffectOnFiber (webpack-internal:///../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:19691:37)
    at commitLayoutMountEffects_complete (webpack-internal:///../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:20844:25)
    at commitLayoutEffects_begin (webpack-internal:///../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:20833:21)
    at commitLayoutEffects (webpack-internal:///../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:20783:13)
    at commitRootImpl (webpack-internal:///../../node_modules/.pnpm/[email protected][email protected]/node_modules/react-dom/cjs/react-dom.development.js:22572:17)

@andlbrei
Copy link
Contributor

andlbrei commented Sep 25, 2024

I have tried to figure out why I still get the null error locally, but not in CSB.
I tried comparing when debugging in CSB vs locally.
I figured out how it fails, and that the behaviour is different, but I don't know what it means/how to fix it.

What I found was that the fields within the PushContainer are set within the updateDataValue function in Provider.js.
In CSB the fields are only iterated over once. Locally they are iterated over twice.
I'm only showing screenshots of the first field, firstName.

The first run locally is equal to the only run in CSB and looks like this:
Note that newData.newItems[0] is an empty Object.
image

On the next pass newData.newItems[0] is null. This is when it fails.
image

@tujoworker
Copy link
Member Author

yes, this was a tricky thing to get right. Now I think we are back on track. Can you test the latest version?

https://pkg.csb.dev/dnbexperience/eufemia/commit/7b3bf06d/@dnb/eufemia

@andlbrei
Copy link
Contributor

No null error now 👏

Still can't remove items.
Also noticed a new (?) problem:
Default value seems to stick.

In our project we toggle between user objects with certain data that is used as default data.
In the case of a new "blank" person the default data isn't set, but if a existing user is selected, and then we switch to new person the defaults are still present.

https://codesandbox.io/p/sandbox/iterate-defaultvalue-debug-dnthhd

@tujoworker
Copy link
Member Author

Lets a have another round of testing 😄 The new version is 6b1f82fa.

Default value seems to stick.

The place where you use defaultValue, I would use just value. A defaultValue can not change, as it is only taken care of on the initial render (almost always, but not in the PushContaine, there it gets re-assigned after the fields have been cleaned).

@andlbrei
Copy link
Contributor

Good point, value is the best choice there.
And it works.
But, it still sticks to fields even if they have no value or defaultValue defined if they share a path.
Dunno how important that is to "fix", it's a strange case.

Removing elements also works now 👍

@tujoworker
Copy link
Member Author

I'm not sure how I can reproduce the sticking issue:

But, it still sticks to fields even if they have no value or defaultValue defined if they share a path.

Let me know more about it.

Can you check if that is an issue in this PR or if it is present in the latest version?

@tujoworker tujoworker mentioned this pull request Sep 30, 2024
@andlbrei
Copy link
Contributor

andlbrei commented Oct 3, 2024

Seems like the issues have been solved now 👍

@tujoworker tujoworker force-pushed the fix/iterate-default-value branch from b04d5b8 to 0f0590b Compare October 5, 2024 08:31
@tujoworker tujoworker force-pushed the fix/iterate-default-value branch from 0f0590b to 9031fec Compare October 5, 2024 08:32
@tujoworker tujoworker merged commit afbdddf into main Oct 5, 2024
10 checks passed
@tujoworker tujoworker deleted the fix/iterate-default-value branch October 5, 2024 09:03
tujoworker pushed a commit that referenced this pull request Oct 11, 2024
## [10.52.0](v10.51.2...v10.52.0) (2024-10-11)

### 📝 Documentation

* **NationalIdentityNumber:** add docs about validation ([#4077](#4077)) ([51bfd80](51bfd80))
* **OrganizationNumber:** adds docs about validation ([#4078](#4078)) ([c785c51](c785c51))

### 🐛 Bug Fixes

* **Autocomplete:** Show whole suggestion list after item selection ([#4060](#4060)) ([0acf061](0acf061))
* **Field.NationalIdentityNumber:** validate on all digits(not only 11 digits) ([#4079](#4079)) ([7c34fc9](7c34fc9))
* **Field.OrganizationNumber:** should validate on all digits(not only when 9) ([#4071](#4071)) ([08a4b51](08a4b51))
* **Forms:** add support for `Form.SubmitConfirmation` inside Wizard ([#4088](#4088)) ([e1167a4](e1167a4)), closes [#4086](#4086)
* **Forms:** don't call internal `exportValidators` when they not are exported as an array ([#4113](#4113)) ([cd54ed0](cd54ed0)), closes [#4106](#4106)
* **Forms:** ensure `emptyValue` is set in the data context when defined ([#4111](#4111)) ([dcc5694](dcc5694)), closes [#4070](#4070)
* **Forms:** ensure `onBlurValidator` gets called when `validateInitially` is true ([#4069](#4069)) ([59cf6c5](59cf6c5)), closes [#4068](#4068) [#4066](#4066)
* **Forms:** ensure Field.SelectCountry has a fallback locale (nb-NO) ([#4114](#4114)) ([568229a](568229a))
* **Forms:** ensure Form.clearData works in React.StrictMode ([#4053](#4053)) ([da0c93a](da0c93a)), closes [#4048](#4048)
* **Forms:** Fix use of unpolyfilled structuredClone in useData hook ([#4108](#4108)) ([1f59f10](1f59f10))
* **Forms:** keep `Iterate.EditContainer` open when falsy value or empty object was given as the iterate value ([#4087](#4087)) ([e932059](e932059)), closes [#4046](#4046)
* **Forms:** show error on every value change when using exported validators ([#4068](#4068)) ([cab6d01](cab6d01)), closes [#4067](#4067)
* **Icon:** icon property typing should accept FormStatus icons ([#4091](#4091)) ([f49eb34](f49eb34))
* **NationalIdentityNumber:** use `onBlurValidator` instead of `validator` ([#3982](#3982)) ([0a93755](0a93755))
* **Typography:** match medium heading font size in Sbanken theme ([#4039](#4039)) ([ce50529](ce50529))

### ✨ Features

* **Forms:** add `bubbleValidation` to Form.Isolation and Iterate.PushContainer to prevent the form from being submitted when there are fields with errors ([#4103](#4103)) ([880f870](880f870)), closes [#4072](#4072)
* **Forms:** add `createMinimumAgeValidator` to Field.NationalIdentityNumber make a customizable `adultValidator` ([#4057](#4057)) ([6c20ba2](6c20ba2))
* **Forms:** add `Form.useSnapshot` hook to handle snapshots of data ([#4102](#4102)) ([d451793](d451793))
* **Forms:** add `isolatedData` prop to `Iterate.PushContainer` ([#4076](#4076)) ([ede5f8e](ede5f8e))
* **Forms:** add `isValid` to Form.Visibility for showing content based on the validation of a field ([#4038](#4038)) ([7536752](7536752))
* **Forms:** add `Iterate.ItemNo` component ([#4095](#4095)) ([c736c9e](c736c9e))
* **Forms:** add `transformLabel` to all Value.* component ([#4056](#4056)) ([d63e472](d63e472))
* **Forms:** add support for `defaultValue` (and `value`) for fields used in `Iterate.Array` ([#3987](#3987)) ([afbdddf](afbdddf)), closes [#3882](#3882)
* **Forms:** add support for `id` for when using dynamic steps with `activeWhen` ([#4093](#4093)) ([248da92](248da92))
* **Forms:** remove internal pattern from `Field.NationalIdentityNumber` in favor of the internal validator ([#4098](#4098)) ([fb35722](fb35722))
* **Forms:** remove internal pattern from `Field.OrganizationNumber` in favor of the internal validator ([#4092](#4092)) ([e829f8b](e829f8b)), closes [#4073](#4073)
* **Skeleton:** stop animation after 30 seconds ([#3479](#3479)) ([f67b3bb](f67b3bb))
* **Value.PhoneNumber and NumberFormat:** displays country code using prefix `+` instead of `00` ([#4051](#4051)) ([fb363d0](fb363d0))
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 10.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Iterate does not use Field.defaultValue/value
3 participants