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

Implement multiple #46

Closed
wants to merge 5 commits into from
Closed

Implement multiple #46

wants to merge 5 commits into from

Conversation

svenvandescheur
Copy link
Collaborator

@svenvandescheur svenvandescheur commented May 12, 2023

Closes #43

@svenvandescheur svenvandescheur force-pushed the feature/#43-input branch 5 times, most recently from 49ee15c to ffdec37 Compare June 1, 2023 15:54
@svenvandescheur svenvandescheur changed the title 🚧 Implement multiple Jun 1, 2023
@svenvandescheur svenvandescheur marked this pull request as ready for review June 1, 2023 16:17
@sergei-maertens sergei-maertens removed their request for review June 1, 2023 17:31
Copy link
Contributor

@CharString CharString left a comment

Choose a reason for hiding this comment

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

If I understand correctly this is not a "container" in the sense of the "edit grid" or "repeating groups". This is the "multiple values" checkbox on any single field component, right?

I got interrupted and now I have the feeling I had a question I can't remember... I guess it will come up if it's important 🤷

src/containers/multiple/multiple.container.tsx Outdated Show resolved Hide resolved
src/containers/multiple/multiple.container.tsx Outdated Show resolved Hide resolved
src/containers/multiple/multiple.container.tsx Outdated Show resolved Hide resolved
Comment on lines 41 to 46
const remove = (index: number) => {
const _keys = keys.filter((_, i) => i !== index);
const val = value.filter((_, i) => i !== index);
setKeys(_keys);
setValue(path, val);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding: we're not allowed to mutate with Array.splice, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd avoid mutating array as much as possible.

src/containers/multiple/multiple.container.tsx Outdated Show resolved Hide resolved
src/lib/renderer/renderer.tsx Show resolved Hide resolved
src/lib/renderer/renderer.tsx Show resolved Hide resolved
src/lib/renderer/renderer.tsx Show resolved Hide resolved
src/lib/renderer/renderer.tsx Show resolved Hide resolved
@svenvandescheur
Copy link
Collaborator Author

svenvandescheur commented Jun 2, 2023

If I understand correctly this is not a "container" in the sense of the "edit grid" or "repeating groups". This is the "multiple values" checkbox on any single field component, right?

I got interrupted and now I have the feeling I had a question I can't remember... I guess it will come up if it's important 🤷

Yes correct, we need to support third party scenario's so we don't want to complicate components too much with this in order. Containers are slightly more tightly coupled with the renderer to take away unwanted complexity.

@svenvandescheur
Copy link
Collaborator Author

svenvandescheur commented Jun 2, 2023

I still have to adjust the validation system to work correctly with multiple values.

@sergei-maertens
Copy link
Member

Sorry, closing this - when we pick up this development again (builder currently has prio since translation management is broken), I will do this via #47 which wipes out most of this implementation path.

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.

Implement input multiple
3 participants