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.

Sign up for GitHub

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

Jump to bottom

fix(forms): add Form.Isolation path support when used inside Form.Section #3829

Merged
merged 5 commits into from
Aug 19, 2024
Merged

fix(forms): add Form.Isolation path support when used inside Form.Section #3829

merged 5 commits into from
Aug 19, 2024

Conversation

tujoworker
Copy link
Member

No description provided.

@vercel Vercel
Copy link

vercel bot commented Aug 15, 2024 •
edited
Loading

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 Aug 16, 2024 5:59pm

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Aug 15, 2024 •
edited
Loading

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.

@tujoworker tujoworker force-pushed the fix/forms-isolation-in-section-support branch from ffbe7c9 to 7f3474c Compare August 16, 2024 11:30
@tujoworker tujoworker requested a review from langz August 16, 2024 11:30
@tujoworker tujoworker force-pushed the fix/forms-isolation-in-section-support branch from 7f3474c to 95bde60 Compare August 16, 2024 11:31
@tujoworker tujoworker force-pushed the fix/forms-isolation-in-section-support branch from 95bde60 to e2a6aa1 Compare August 16, 2024 12:25
@tujoworker tujoworker requested a review from andlbrei August 16, 2024 12:29
langz
langz approved these changes Aug 16, 2024
View reviewed changes
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.

Looks good to me, thanks for the demo 🍿

@andlbrei
Copy link
Contributor

There is something weird happening, and I'm not sure exactly how.
The isolated value ends up on the root of the committed object.

My.Movie.mp4

Also, might be unrelated to this MR and not necessarily a problem, but I would have thought that the onCommit data parameter would only contain the object that is in the scope of the Form.Isolation component.
And perhaps the same for onPathChange.
At least that's how I think about this component. It is isolated from the rest of the form, doesn't know or care what the parent paths are, and the data will be committed to the place it belongs to in the hierarchy, which the parent has control over (or somewhere else if it is overriden via props).
This is a bit taxing to think about, so I might be completely off base, missing some cruical reasons as to why it can't be, or shouldn't be, like this 😅

<Form.Section path="/mySection">
  <Field.String path="/name" />
  <Form.Isolation onCommit={(data) => console.log(data)}> // Will log { address: "Some user input" } and inserted/merged into { mySection } resulting in { mySection: { name: "Some name", address: "Some user input" }}
    <Field.String path="/address" />
  </Form.Isolation>
</Form.Section>

@tujoworker tujoworker force-pushed the fix/forms-isolation-in-section-support branch from 514feb2 to 8b41c22 Compare August 16, 2024 14:38
@tujoworker
Copy link
Member Author

yes, very good points 💯 Both of your concerns are implemented now. Thank you @andlbrei 🥇
Happy if you can verify it and eventually approve the PR ✅

@andlbrei
Copy link
Contributor

@tujoworker If the user presses commit without doing any changes, then the default value won't be commited.

Other than that it seems good 👌

@tujoworker
Copy link
Member Author

Ohh yes. The latest commit fixes this issue, including a test.

andlbrei
andlbrei approved these changes Aug 19, 2024
View reviewed changes
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.

👌

@tujoworker tujoworker merged commit d836c18 into main Aug 19, 2024
10 checks passed
@tujoworker tujoworker deleted the fix/forms-isolation-in-section-support branch August 19, 2024 07:52
tujoworker pushed a commit that referenced this pull request Aug 19, 2024
@semantic-release-bot
chore(release): 10.44.0 [skip ci]
b45162a 
## [10.44.0](v10.43.0...v10.44.0) (2024-08-19)

### 📝 Documentation

* **Field.Expiry:** display translation props ([#3815](#3815)) ([1efbbd0](1efbbd0))
* **Field.PhoneNumber:** document onChange event args ([#3807](#3807)) ([2b5f55e](2b5f55e))
* **Form.Isolation:** add real world examples ([#3808](#3808)) ([daec2fa](daec2fa))

### 🐛 Bug Fixes

* **Button:** onClick can now be set using context provider ([#3781](#3781)) ([0d2f525](0d2f525))
* **forms:** add `Form.Isolation` path support when used inside `Form.Section` ([#3829](#3829)) ([d836c18](d836c18))
* **forms:** ensure correct spacing between `Iterate.Array` items ([#3838](#3838)) ([250aec0](250aec0))
* **forms:** ensure Form.Isolated does merge data properly ([#3811](#3811)) ([8c90cb8](8c90cb8)), closes [#3806](#3806)
* **forms:** ensure Form.Isolation with commitHandleRef triggers field errors ([#3810](#3810)) ([204396d](204396d))
* **forms:** show error when entered `Field.Currency` or `Field.Number` value exceeds maximum possible amount ([#3821](#3821)) ([e9cdd68](e9cdd68))

### ✨ Features

* **Field.PhoneNumber, Field.SelectCountry:** additional event args for onFocus and onBlur ([#3820](#3820)) ([1da5d4f](1da5d4f))
* **Field:** adds typing for event callbacks ([#3830](#3830)) ([2275002](2275002))
* **Form:** continuousValidation did not update error state correctly ([#3804](#3804)) ([5ff74bd](5ff74bd))
* **forms:** add `dataPath` prop to Field.Selection ([#3816](#3816)) ([e43ad59](e43ad59))
* **forms:** add `Form.clearData` method to remove existing/shared data forms with an id ([#3809](#3809)) ([acf4c7f](acf4c7f)), closes [#3803](#3803) [/github.com//issues/3803#issuecomment-2283661946](https://github.com/dnbexperience//github.com/dnbexperience/eufemia/issues/3803/issues/issuecomment-2283661946)
* **forms:** add `transformOnCommit` to Form.Isolation ([#3813](#3813)) ([21eca1a](21eca1a)), closes [#3812](#3812) [#3808](#3808)
* **forms:** add path prop support to Form.Isolation ([#3812](#3812)) ([a16d782](a16d782)), closes [#3811](#3811)
* **Forms:** seperate translations for Boolean and Toggle ([#3819](#3819)) ([fa0827b](fa0827b))
* **Upload:** enables disabling of fileMaxSize ([#3835](#3835)) ([f2eff23](f2eff23))
@tujoworker
Copy link
Member Author

🎉 This PR is included in version 10.44.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
Reviewers

@langz langz langz approved these changes

@andlbrei andlbrei andlbrei approved these changes

Assignees
No one assigned
Labels
released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tujoworker @andlbrei @langz