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

Autocomplete field does not work with on('change', $form->js->form(submit)) - triggers twice #321

Closed
PhilippGrashoff opened this issue Jan 11, 2018 · 15 comments
Assignees
Labels
hangout agenda 🔈 Will discuss on next hangout JS

Comments

@PhilippGrashoff
Copy link
Collaborator

PhilippGrashoff commented Jan 11, 2018

If the Autocomplete field is used with on change event listener to submit the form, the form gets submitted twice if the search field is used. See this screencast:

  1. At the start, I select a record from the list, the form submit is correct: Its submitted once and the ID is passed.
  2. After, I use the text input to search for a record. If I select one then, the form is submitted twice, one time with the old ID, one time with the new ID.

see: http://www.spame.de/autocompletebug.mkv

The code for this is:


<?php

//Load Main App
require'lib.php';

$app = new \Pmg\TourManagement();

$f = $app->layout->add('Form');

$autocomplete_model_guide = new \Pmg\Guide($app->db);

$r = $f->addField('add_guide_to_tour', [
	'AutoComplete',
	'model'			=> $autocomplete_model_guide,
	'placeholder'	=> 'Bestehenden Guide hinzufügen',
	'search'		=> ['name'],
]);
$r->on('change', $f->js()->form('submit'));
$f->onSubmit(function() use ($f, $r) {
	echo $r->getValue();
});

Best regards
Philipp

@ibelar
Copy link
Contributor

ibelar commented Jan 16, 2018

Couple of things happen here.
First returning Echo seem to jeopardize the Autocomplete field, would be better off simply returning a notifier instead:

$f->onSubmit(function() use ($f, $r) {
	return new atk4\ui\jsNotify($r->getValue());
});

Even returning a notifier, form will be submit many time with the way the basic AutoComplete is define. Every time a user will use the up or down arrow key on their keyboard, it will fire the submit event. Plus, the submit event will also fire on blur event, i.e. when getting away from the field.

AutoComplete is based on semantic-ui Dropdown module. For better control on the 'change' event, I would redefine the semantic-ui Dropdown module options and adjust accordingly to your need by setting the onChange handler.

Here is how to:

  • Create a javascript file, let say, my-autocomplete.js
  • Redefine semantic-ui dropdown settings to your need inside my-autocomplete.js file like so:
var myAcPreviousValue = 0;
//this will fire onChange callback when true;
$.fn.dropdown.settings.allowReselection = true;

//this will prevent onChange from firing if user is selecting entry using up / down arrow key.
$.fn.dropdown.settings.selectOnKeydown = false;

//Will fire when user choose a value.
$.fn.dropdown.settings.onChange = function(value,t,c) {
 // check if value has been submit already.
  if (value !== myAcPreviousValue) {
    $(this).parents('.form').form('submit');
    myAcPreviousValue = value;
  }
};
  • in your php file, simply load the js file using:
$app->requireJS('my-autocomplete.js');
  • Do not forgot to remove your previous change event handler.
//remove this
//$r->on('change', $f->js()->form('submit'));

@PhilippGrashoff
Copy link
Collaborator Author

Hi Ibelar,
thanks for that fix!
I wonder how we could implement this in ATK. As now, this affects all dropdowns, though its only needed for the Autocomplete And all the other onChanges are defined from PHP.

A quick idea, but I am no JS expert... could this work? Just add all this JS in AutoComplete->init(), only for each individual Autocomplete/all Autocompletes?

Best regards
Philipp

@ibelar
Copy link
Contributor

ibelar commented Jan 16, 2018

Sure can. You can extends AutoComplete and pass your own options in init() method:

function public init()
{
       //previous code here...
        $chain->dropdown([
            'fields'      => ['name' => 'name', 'value' => 'id'/*, 'text' => 'description'*/],
            'apiSettings' => array_merge($this->apiConfig, [
                'url' => $this->getCallbackURL().'&q={query}',
            ]),
            'allowReselection' => true,
            'selectOnKeydown'  => false,
            'onChange' => new jsExpression('function(value,t,c){
              if ($(this).data("value") !== value) {
                $(this).parents(".form").form("submit");
                $(this).data("value", value);
              }
            }')
        ]);
  //remaining code here
}

@romaninsh
Copy link
Member

@ibelar maybe we should move $chain->dropdown into a separate method, so that users could override that if they wanted to?

@ibelar
Copy link
Contributor

ibelar commented Jan 17, 2018

Good idea. let me check on that.

@PhilippGrashoff
Copy link
Collaborator Author

Hi ibelar,

did you test the last code you posted? I tried it, but for me it does not work.
I extended the AutoComplete and added the code you posted inside init() (which seems very reasonable by the way), but it does not change the behaviour of the field at all.

Adding the JS you posted in #321 (comment) works perfectly on the contrary.

@ibelar
Copy link
Contributor

ibelar commented Jan 26, 2018

Philipp;

Code has change since the last time. You can now pass the dropdown setting directly

$form->addField('field', ['AutoComplete', 'settings'=>['allowReselection' => true,
                            'selectOnKeydown' => false,
                            'onChange'        => new atk4\ui\jsExpression('function(value,t,c){
                                                           if ($(this).data("value") !== value) {
                                                             $(this).parents(".form").form("submit");
                                                             $(this).data("value", value);
                                                           }
                                                          }'),
                           ],
                           'model'       => $my_model;

Something like above should work, passing the settings array to Addfield.

Or look at the new Autocomplete::initDropdown($chain) method to achieve what you need.

hope this help;

@PhilippGrashoff
Copy link
Collaborator Author

Hi,

this works perfect, tanks for all the JS help!

Best reagards

@romaninsh
Copy link
Member

What's the outcome of this ticket? invalid? not really a bug? Needs enhancement somewhere?

@PhilippGrashoff
Copy link
Collaborator Author

IMO its a bug, but caused by Semantic-UI JS as far as I understand.

The last fix Ibelar posted works perfectly in terms of functionality.

Im terms of coding, creating the onChange event differs:

  • for every other input, its $input->on('change', function () {...});
  • the current solution for Autocomplete passes the working onChange as part of the settings-parameter/property.

To keep ATK UI coding style consistent, creating this functionality via $autocomplete->on('change'.....
would be good... the best solution anyway would be if the basic JS used would work the way it should :)

Best regards
Philipp

@romaninsh
Copy link
Member

we can technically override on() method.............

@PhilippGrashoff
Copy link
Collaborator Author

read into Semantic UI a bit yesterday, there was hardly any development for over a year but its starting to roll again. Perhaps it will be fixed this way? I didn't try to find a corresponding issue in Sematic UI as there is over 1000 open issues...

@romaninsh
Copy link
Member

:) yeah, if it's not a big trouble, send Jack a donation, i'm doing that monthly and I think it's great encouragement. Our project relies on Semantic UI heavily so we need them to keep going. Button is at the bottom of the semantic-ui.com page.

@PhilippGrashoff PhilippGrashoff added the hangout agenda 🔈 Will discuss on next hangout label Aug 22, 2019
@PhilippGrashoff
Copy link
Collaborator Author

lets check in hangout if this issue can be closed, there were many changes introduced in Fomantic-UI to DropDown.

@ibelar
Copy link
Contributor

ibelar commented Sep 20, 2019

AutoComplete::onChange and DropDown::onChange is now working. Closing this.

@ibelar ibelar closed this as completed Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hangout agenda 🔈 Will discuss on next hangout JS
Projects
None yet
Development

No branches or pull requests

3 participants