-
Notifications
You must be signed in to change notification settings - Fork 33
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 Iterate.Array feature and merge useField and useValue to one hook useDataValue #2635
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 🚀 – just some comments ✨
...s/dnb-design-system-portal/src/docs/uilib/extensions/forms/advanced-blocks/Iterate/Array.mdx
Show resolved
Hide resolved
...gn-system-portal/src/docs/uilib/extensions/forms/advanced-blocks/Iterate/ArrayPushButton.mdx
Show resolved
Hide resolved
...gn-system-portal/src/docs/uilib/extensions/forms/advanced-blocks/Iterate/ArrayPushButton.mdx
Show resolved
Hide resolved
...gn-system-portal/src/docs/uilib/extensions/forms/advanced-blocks/Iterate/ArrayPushButton.mdx
Show resolved
Hide resolved
...-portal/src/docs/uilib/extensions/forms/advanced-blocks/Iterate/ArrayRemoveElementButton.mdx
Show resolved
Hide resolved
packages/dnb-eufemia/src/extensions/forms/Iterate/__tests__/Array.test.tsx
Show resolved
Hide resolved
packages/dnb-eufemia/src/extensions/forms/Iterate/ArrayRemoveElementButton.tsx
Outdated
Show resolved
Hide resolved
packages/dnb-eufemia/src/extensions/forms/Iterate/ArrayRemoveElementButton.tsx
Outdated
Show resolved
Hide resolved
packages/dnb-eufemia/src/extensions/forms/Iterate/ArrayPushButton.tsx
Outdated
Show resolved
Hide resolved
const iterateElementContext = useContext(IterateElementContext) | ||
const { handlePush } = iterateElementContext ?? {} | ||
|
||
const { text, value, pushValue, handleChange, children } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { text, value, pushValue, handleChange, children } = | |
const { text, value, pushValue, handleChange, children, ...rest } = |
So we can send along HTML attributes like aria-*.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could mix things up a bit, since useDataValue
adds a few optional props and callbacks, such as onFocus
, which would then end up being sent to Button
. There is also optional callbacks like handleChange
and onChange
in separate callbacks (explained in code comments) which would both be sent to Button
. I would suggest extending some function to pick area-tags or similar sets of properties like it is done for spacing props here, to ensure only intended props are passed into sub components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of a discussion between whitelisting and blacklisting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my experience, we NEED to allow everything to be forwarded to the button. In that case we need to know all the properties from a nested component and remove them, or destruct them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could forward everything, but then it would have to be from the received props (direct to the component, not those received through useDataValue
), and then override the specific props from useDataValue
that we know it will have overridden, like the click event. Otherwise the Button will receive a lot of props from logic inside useDataValue
that is not made for the Button component. It might ignore them, but it might be similar names intended fir different use cases, which could lead to problems. However, if the ArrayPushButton extend the props of Button, and pass them directly (not through the hook) and just use specific props from the hook, it will not cause that problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Sure, that's right. I probably did comment on the wrong place about the rest props 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed now ✅
ad80ce3
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. Latest deployment of this branch, based on commit 564d6e4:
|
2e8c4d8
to
34aaeca
Compare
packages/dnb-design-system-portal/src/docs/uilib/extensions/forms/advanced-blocks/Iterate.mdx
Show resolved
Hide resolved
Seems to be a slight "vertical misalignment" between the input field and the button in one of the I was able to see the "same" in the "Array demo With DataContext and add/remove buttons" https://eufemia-git-feat-forms-array-eufemia.vercel.app/uilib/extensions/forms/advanced-blocks/Iterate/Array/demos/#with-datacontext-and-addremove-buttons |
I seem to get quite a lot of issues/warnings in the console with labels that are not connected to an existing id of "form field". Quite a few of the issues/warnings is fixed as part of #2634, so please rebase with latest main, but I do see a few of them(like "Accounts" in the screenshot above) coming from this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I've added a few comments that could be worth looking into.
Great work 🚀 🎸
73dbb33
to
ad80ce3
Compare
@@ -1,4 +1,4 @@ | |||
### Standard input component props | |||
### Standard data value component props | |||
|
|||
| Property | Type | Description | | |||
| ---------------------- | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to have a specific prop for data-testid
(it's listed as a property in this file)? 😅
Or can we remove it?
Also opened a related PR: #2641
I was not able to comment on the line below, hence commenting here.
The first one (Remove avenger buttons) is because they are put horizontally using In the second example you sent, it is centered in a row containing the name/birth year inputs, but not the one below (with Nick Fury), so it has produced the expected result: |
ad80ce3
to
3a6cb54
Compare
2a535eb
to
564d6e4
Compare
## [10.9.0](v10.8.0...v10.9.0) (2023-09-13) ### 📝 Documentation * **Dialog:** Dialog.Actions to Dialog.Action ([#2621](#2621)) ([9292305](9292305)) * **Forms:** restructure docs ([#2615](#2615)) ([ee24eba](ee24eba)) * **Modal:** updates components that inherits ModalPropTypes as camel case ([#2618](#2618)) ([beacaba](beacaba)) * **PaymentCard:** removes unused maker docs ([#2619](#2619)) ([dfeb6d5](dfeb6d5)) * **v10:** changes to PaymentCard's height & width ([#2643](#2643)) ([6985287](6985287)) ### ✨ Features * **Blockquote:** fix sbanken styling, added small screen version ([#2639](#2639)) ([d63d30d](d63d30d)) * **Forms:** add `Form.Handler` component ([#2644](#2644)) ([82873c7](82873c7)) * **Forms:** add Iterate.Array feature and merge useField and useValue to one hook useDataValue ([#2635](#2635)) ([b056994](b056994)) * **Forms:** merge FieldGroup into FieldBlock ([#2645](#2645)) ([56a1867](56a1867)) * **Heading:** remove font ligatures for sbanken headings ([#2614](#2614)) ([06c126f](06c126f)) * **PaymentCard:** updates height and weight to reflect Figma sketches ([#2617](#2617)) ([418fd2e](418fd2e)) * **ProgressIndicator:** Sbanken styling ([#2631](#2631)) ([03de0e3](03de0e3)) * **Skeleton:** Sbanken styling ([#2622](#2622)) ([fbc0083](fbc0083)) * **Tooltip:** Sbanken styling ([#2640](#2640)) ([f358b0a](f358b0a)), closes [#2622](#2622) [#2631](#2631) ### 🐛 Bug Fixes * **Dialog:** select whole input on focus given by focusSelector ([#2655](#2655)) ([a04f99a](a04f99a)), closes [#2652](#2652) * **Forms:** add polyfill for structuredClone ([#2636](#2636)) ([f00da48](f00da48)) * **Forms:** focus InputMasked when clicking label ([#2632](#2632)) ([3233326](3233326)) * **Forms:** remove FirstName, LastName and InfoCardSection ([#2627](#2627)) ([a2106cc](a2106cc)) * **MaskedInput:** make cleanedValue contain leading zeroes ([#2610](#2610)) ([86ad122](86ad122)) * **ProgressIndicator:** ensure correct positioning inside its boundary ([#2623](#2623)) ([0592961](0592961)) * **Section:** omit usage of nullish operator (??) to still support Storybook v4 ([#2646](#2646)) ([9a2e52f](9a2e52f)) * **useMedia:** ensure this hook works in StrictMode ([#2630](#2630)) ([74ebed9](74ebed9))
🎉 This PR is included in version 10.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Dynamic data handler component for arrays, making it simple to render fields on the screen based on a varying number of elements in a data set consisting of array elements.