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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize validation.js performance #4435

Merged
merged 8 commits into from Jan 16, 2015

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Oct 2, 2014

it a try to optimise the Joomla! form rendering with a lot inputs.
validation.js it third slow script after chosen.js and bootstrap.tooltip on the form page,

Here is some graphs.
test made with disabled chosen.js and bootstrap.tooltip on Global configuration page.

Before this fix:
screen 2014-10-02 16 23 30 1164x438
After fix:
screen 2014-10-02 16 19 55 1085x445

As can see difference around 400ms for JFormValidator initialization, at least on my PC in Chrome 馃槃

How to test.
Apply patch.
Go to global configuration, or other form where a lot inputs, and compare whether all still works and whether it "feels faster" 馃槈

@anibalsanchez
Copy link
Contributor

@test OK


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4435.

@zero-24
Copy link
Contributor

zero-24 commented Jan 14, 2015

@Fedik can you fix the merge conflicts here? I will test after a resync to be sure that nothing is broken 馃槃 Thanks!

@Fedik
Copy link
Member Author

Fedik commented Jan 15, 2015

@zero-24 done 馃槈

and I added strict mode, so please retest

@zero-24
Copy link
Contributor

zero-24 commented Jan 15, 2015

Thanks @Fedik

@test successful I can't see any issues with the patch applyed.

and I added strict mode, so please retest
@Fedik do @anibalsanchez also retest or can we move it now to RTC?

@Fedik
Copy link
Member Author

Fedik commented Jan 15, 2015

yes, would be good to retest 馃槈

@anibalsanchez can you please try one more time? 馃槈

@anibalsanchez
Copy link
Contributor

... hmmm ... I have manually copied the two files... In Global Configuration, I try to save an empty Site Name, it shows the message "Invalid field: Site Name"... Cancel button does not work.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4435.

@Fedik
Copy link
Member Author

Fedik commented Jan 15, 2015

@anibalsanchez I just checked without the patch, and got same result as you,
so I think it is not a regression that made by this pull request ....
looks like you found some bug in global configuration page 馃槃

@Fedik
Copy link
Member Author

Fedik commented Jan 15, 2015

yes it 馃悰 on the configuration page,
there test for task == "application.cancel" but must be task == "config.cancel.application" on first look

@zero-24
Copy link
Contributor

zero-24 commented Jan 15, 2015

@Fedik do you think we should fix this with this pull or should we open a new for this issue?

@dgrammatiko
Copy link
Contributor

@Fedik You are right here: cancel button has <button onclick="Joomla.submitbutton('config.cancel.application')" class="btn btn-small">

and the script we inject is :

    Joomla.submitbutton = function(task)
    {
        if (task == "application.cancel" || document.formvalidator.isValid(document.getElementById("application-form")))
        {
            Joomla.submitform(task, document.getElementById("application-form"));
        }
    };

@Fedik
Copy link
Member Author

Fedik commented Jan 15, 2015

@zero-24 I think would be better separate as it is different things .. I can try make another pull with fix, but at evening

@zero-24
Copy link
Contributor

zero-24 commented Jan 15, 2015

@zero-24 would be better separate as it is different things .. I can try make another pull with fix, but at evening

Thanks moving here to RTC as the other issue will be handled in a seperate PR by @Fedik Thanks to all!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4435.

@dgrammatiko
Copy link
Contributor

@test success
before time = 1.004 ms
screen shot 2015-01-15 at 3 36 39

After time = 0.734 ms
screen shot 2015-01-15 at 3 37 22

@Fedik
Copy link
Member Author

Fedik commented Jan 15, 2015

there fix for cancel button #5738

@brianteeman brianteeman added the RTC This Pull Request is Ready To Commit label Jan 16, 2015
roland-d added a commit that referenced this pull request Jan 16, 2015
@roland-d roland-d merged commit 2d80b07 into joomla:staging Jan 16, 2015
@roland-d roland-d added this to the Joomla! 3.4.0 milestone Jan 16, 2015
@Fedik Fedik deleted the validationjs-performance branch January 17, 2015 09:57
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
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.

None yet

7 participants