[#5047] - Introduce jquery formvalidator for com_menus
- Closed
- 2 Dec 2014
- Medium
- Build: staging
- # 5047
- Diff
- dgt41:_form_jq_com_menus
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Executive summary
This PR converts the form validation on com_menus to use plain jquery (no mootools call on every form).
Also NO MORE INLINE SCRIPTS!
Testing
- Apply first PR #4888 (!important)
- Apply this PR
- In the admin area go to com_menus and try to submit any form.
If no javascript errors are logged in your browser and the functionality remains the same your test is a pass in any other case please report the errors here
Please also check these:
administrator/index.php?option=com_checkin should demonstrate multiselect without mt
administrator/index.php?option=com_users&view=mail should demonstrate form sent and validate without mt
administrator/index.php?option=com_modules should demonstrate multiselect and combobox without mt
http://localhost/administrator/index.php?option=com_admin&view=sysinfo should demonstrate highlighter.js without mt
Logout and log in to demonstrate the use of noframes without mt.
jissues-bot
- | Labels |
Added:
?
|
||
brianteeman
- | Category | ⇒ | JavaScript |
| Status | Pending | ⇒ | Ready to Commit |
Moving to RTC as we have 2 successful tests.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5047.
| Labels |
Added:
?
|
||
@phproberto Also this one needs review dgt41@f26d63f
when tried to set a menu type Uncaught ReferenceError: setmenutype is not defined
also syntax error Unexpected token ) when modal opens
looks like method setmenutype in different scope or because syntax error 
| Labels |
Removed:
?
|
||
There were changes here. Changing status to not RTC
| Labels |
Added:
?
|
||
@infograf768 Yes 3 views had to have some changes so we will end up with a consistent way of adding javascript code. This is what @phproberto asked in #5058. So I just updated this and #5046 to fulfill this request. The last commit in both indicates the changes!
| Status | Ready to Commit | ⇒ | Pending |
Set status back to Pending as there are changes that needs testing.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5047.
@test success, on a brand new staging installation / diff can't be applied on J 3.3.6
Works much better than previous menu validation.
- Tested trying to create an empty menu item, validation OK
- Only filling a menu name, validation OK
- Only filling a menu name and menu type assignment, validation OK
Minor issue, if Menu Item Type="Single Article", and article is not selected, it detects the field as mandatory, but the message does not show the field name: "Error Invalid field: "
@anibalsanchez It is normal that will not apply to 3.3.6 because there was another PR #4517 from @Fedik Actually the Kudos here should go all to @Fedik I just wrapped it to <head>
Minor issue, if Menu Item Type="Single Article", and article is not selected, it detects the field as mandatory, but the message does not show the field name: "Error Invalid field: "
As I understand it related to validate.js ... the validation script cannot find the field label in some reason,
I think it not related to the current pull request, as it do nothing with it
@test successful. No JS errors and all forms work as expected.
Moving to RTC as we have 2 successful tests.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5047.
| Status | Pending | ⇒ | Ready to Commit |
I would wait until someone confirm that wrap a simple function definition into jQuery.ready is a good idea.
Currently I see next potential b/c issue:
The extension use ready event for override Joomla.submitbutton, but as we also wrap it into ready the extension can stop working, because no guarantee that our Joomla.submitbutton will be defined before the override from the extension.
@dgt41 I mean next case:
//extension code
jQuery(document).ready(function() {
// this will be defined first
Joomla.submitbutton = functin(){ /* do something*/ }
});
// joomla code
jQuery(document).ready(function() {
// this will override
Joomla.submitbutton = ....
});such thing can happens if extension call addScriptDeclaration($script) before it was called in our code.
and when we use it without ready:
// extension code
jQuery(document).ready(function() {
// this will override
Joomla.submitbutton = functin(){ /* do something*/ }
});
// joomla code
// this will be defined first
Joomla.submitbutton = ....
I see your point, but I think is invalid because extensions code is always overriding the code in core.js. Theoretically for the ms that the core.js code is executed and our code that is wrapped in .ready() YES we have a problem, but in practice this will never happen because also the DOM is not complete, so no kind of interaction can take place in a blank page. Am I wrong?
I mean Joomla.submitbutton that added in menus/tmpl/default.php that already override the same function from core.js 
never happen because also the DOM is not complete
ok, try execute next code (just copy in the index.php in <script> tag, or in simple html file and run):
var myCoolFunction;
// must be override (eg Extension override Joomla.submitbutton)
jQuery(document).ready(function() {
myCoolFunction = functin(){ console.log('I am override!'); }
});
// must be initial definition (eg Joomla.submitbutton code)
jQuery(document).ready(function() {
myCoolFunction = functin(){ console.log('I am the first of the firsts!'); }
});
// run the function
jQuery(document).ready(function() {
myCoolFunction();
});and then another:
var myCoolFunction;
// must be override (eg Extension override)
jQuery(document).ready(function() {
myCoolFunction = functin(){ console.log('I am override!'); }
});
// must be initial definition (eg Joomla code)
myCoolFunction = functin(){ console.log('I am the first of the firsts!'); }
// run the function
jQuery(document).ready(function() {
myCoolFunction();
});what you got?
Expected result is I am override! in browser console.
Hmmm I see your point
First example should be:
var myCoolFunction;
// must be initial definition (eg Joomla.submitbutton code)
jQuery(document).ready(function() {
myCoolFunction = function(){ console.log('I am the first of the firs!'); }
});
// must be override (eg Extension override Joomla.submitbutton)
jQuery(document).ready(function() {
myCoolFunction = function(){ console.log('I am override!'); }
});
// run the function
jQuery(document).ready(function() {
myCoolFunction();
});
But again the initial Joomla code is not wrapped in .ready() so will never happen. Also I cannot imagine declaring multiple times the submit button in one component. Am I just thinking too narrow?
First example should be
yes right, but if the extension (I mean 3p extensions) run addScriptDeclaration before it executed in menus/tmpl/default.php then we will get a wrong result.
But again the initial Joomla code is not wrapped in .ready() so will never happen
no matters where was initial, in javascript you can override it 1000 times
In my example replace var myCoolFunction to:
var myCoolFunction = function(){console.log('I am initial')};it will demonstrate initial
maybe I just bad explained
@Fedik A race condition is very rare in this com_menus case. If we expect several competing submitbutton functions, all functions would have to be ready to call a previous submitbutton to avoid conflicts and run all of them.
In practice, components just assign Joomla.submitbutton at any time. I think it is safe to keep with the current style.
@Fedik @anibalsanchez Stupid question but I have to ask: In 3.3.6 if you were about to override the joomla.submitbutton how will you do it? You have the option to add the script through addScriptDeclaration() but it will end up in <head> and the current code is inline script. So in 3.3.6 you had to make sure that DOM was parsed before you replace the function, so you have to use .ready() or domready on mootools. Am I right on this? If that’s the case then I don’t see a problem here
@dgt41 in 3.3.6 you can make it in the custom field or in the system plugin, using addScriptDeclaration
had to make sure that DOM was parsed
not really, you had to make sure that your override will be after the Joomla definition
the browser execute javascript when the parser found it, no matter whether DOM fully parsed or not
@anibalsanchez I just not very liked idea: wrap a simple function definition into jQuery.ready
Honestly I am confused here. We have the initial definition in core.js and then we override it (either with .ready() or without, in this particular occasion with .ready(). Is there a reason to override it again? This is what are we talking about? Or the case that core.js will not be overridden?
In my opinion, it is perfectly fine to override Joomla.submitbutton in any way, either in plain inline script, in a JS file, or using .ready().
There is no other style guideline or general definition.
@smanzi @anibalsanchez @Fedik Can you verify that nothing is broken here?
test
it still works
all good, but
jQuery(document).ready(function (){
Joomla.submitbutton = function(task, type){
....
still here 
test
all good
You still have jQuery(document).ready(function ($){ at line 27 of administrator/components/com_menus/views/item/tmpl/edit.php - Expected?
.. and item/tmpl/edit_modules.php
Sorry carried away
my fault! 
@anibalsanchez I just tested applying this PR via the command line and no errors there. Still a mystery :)
zero-24
- | Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-12-02 20:08:33 |
Merged into staging. Thanks!
| Labels |
Removed:
?
|
||
@dgt41
Unable to test together with #4661
Tested alone, unable to select Menu Item Type from modal