-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix loading new item list after adding new created element on sortable collections #8202
base: 4.x
Are you sure you want to change the base?
Conversation
…#8201 Apply fix as proposed in sonata-project#8201 (comment)
…e collections sonata-project#8201 Apply fix as proposed in sonata-project#8201 (comment)
var selections = jQuery('#{{ id }}').val().split(','); | ||
selections.push(data.objectId); | ||
jQuery('#{{ id }}').val(selections.filter((val) => val.length > 0).join(',')); |
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.
If entities use string as id, the usage of ,
could conflict no ?
if (str_contains($data[0], ',')) { | ||
$event->setData(explode(',', $data[0])); |
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.
While it's work for your usage (with the javascript),
I assume it will introduce a bug if someone use this Extension/FormType in another situation for texte with string like
['Foo, bar']
It will become ['Foo', 'bar']
instead of staying the same...
I dunno if there is a way to avoid such thing...
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.
I think the correct approach here would be to have multiple hidden inputs, one for each id. Like the ModelAutocompleteType does it.
This would probably be more work, tho.
Another approach would be adding a special prefix to the value, that then can be checked.
Co-authored-by: Vincent Langlet <[email protected]>
Subject
This Pull request closes two issues related to sortable collections as described in #8201
I am targeting this branch, because this patches an existing issue.
Closes #8201
Changelog