Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[4.1] Allow passing parent form fields along with inline create dialog request #3013

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Jul 8, 2020

Allow the devloper to setup:

....
 'inline_create'     => [
                    'entity'      => 'entity',
                    'modal_class' => 'modal-dialog modal-xl',
                    'include_main_form_fields' => ['field1','field2']
                ],
....
 'inline_create'     => [
                    'entity'      => 'entity',
                    'modal_class' => 'modal-dialog modal-xl',
                    'include_main_form_fields' => true //all form fields
                ],
....
 'inline_create'     => [
                    'entity'      => 'entity',
                    'modal_class' => 'modal-dialog modal-xl',
                    'include_main_form_fields' => 'only_one_field'
                ],

By default it would be false.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@tabacitu
Copy link
Member

tabacitu commented Jul 9, 2020

Awesome! This fixes #2912 . Merge into master, it'll be available later today with a composer update. Thanks a lot @dividy & @pxpm !

@tabacitu
Copy link
Member

tabacitu commented Jul 9, 2020

@pxpm I just merged this BUT I think there's problem here, judging from the docs, so I reverted the merge.

We now have two attributes which look very similar - include_all_form_fields and include_main_form_fields. And after this PR it looks like include_all_form_fields no longer does anything. Not sure it did do anything beforehand though.

We need to find a way to make both of them make sense, if they both exist. And keep this change backwards-compatible - that's a MUST. Can you please investigate how we can do that?

To me it looks like before this PR, include_all_form_fields was true by default. That means we need to keep this behaviour for now - only in 4.2 can we change the default to false.

We need a different PR that brings this change (and fixes the above), since I merged this PR, but then reverted the merge.

@pxpm
Copy link
Contributor Author

pxpm commented Jul 9, 2020

Indeed @tabacitu there are two attributes very similar but they are not related.

include_all_form_fields is part of the field ajax, it's present in select_from_ajax too, it is the way developer can pass the form fields when we do the search in the field.

I did not changed nothing related to that. It's still true by default (but planned to be false on 4.2).

It's used like I said with the ajax search inside field.

var performAjaxSearch = function (element, $searchString) {
    var $includeAllFormFields = element.attr('data-include-all-form-fields')=='false' ? false : true;
....

Different is include_main_form_fields that belongs to the inline create world. That allow the developer to sent the parent fields to the inline create modal request endpoint. This is a new feature so I started with false by default.

I think you misread something here, or I did not make myself clear. I did not touch nothing related to include_all_form_fields.

$this->crud->addField([
....
'include_all_form_fields' => true,
'inline_create' => [
    'include_main_form_fields' => 'field'
]

I agree that we should allow the same functionality in both of them (the later allows to pass one, multiple or all fields, while our previous solution only allow all fields or none), but as that would be a breaking change I did not touch it.

@tabacitu
Copy link
Member

tabacitu commented Jul 9, 2020

Ah ok, I get it now, thanks for the clarification @pxpm ! My bad!

I makes sense to have both then, I agree, I've reverted the revert 😆 in #3023 so indeed it'll be available in the next patch update.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants