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

Adding additional support for polyfills in Zikula core #2377

Merged
merged 19 commits into from
Mar 30, 2015
Merged

Adding additional support for polyfills in Zikula core #2377

merged 19 commits into from
Mar 30, 2015

Conversation

shefik
Copy link
Contributor

@shefik shefik commented Mar 29, 2015

Adding additional support for polyfills in Zikula core. Also, enabling polyfill in the User Registration form.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2321
Refs tickets #2373
License MIT
Doc PR -
Changelog updated yes

@shefik
Copy link
Contributor Author

shefik commented Mar 29, 2015

This pull request furthers the polyfill solution that was introduced in the Zikula installer, so now the entire Zikula core can utilize it more easily/effectively via PageUtil.

Also, this pull request specially enables the polyfill in the User Registration form, which means that HTML5 form validation is enabled cross-browser. Right now, the code is properly handling the "required" attribute which is in the template. This means, we don't need new classes for error handling, such as "validation-error". We also don't need a "Check your entries" button, since the form validation will be handled natively via the browser's native HTML5 support.

I have more code to commit shortly, which will handle other validations from zikulausersmodule_ajax_getregistrationerrors.

@shefik shefik changed the title Adding additional support for polyfills in Zikula core. [WIP] Adding additional support for polyfills in Zikula core Mar 29, 2015
@Guite Guite added this to the 1.4.0 milestone Mar 29, 2015
@shefik shefik changed the title [WIP] Adding additional support for polyfills in Zikula core Adding additional support for polyfills in Zikula core Mar 29, 2015
@shefik
Copy link
Contributor Author

shefik commented Mar 29, 2015

I have completed the code for this pull request, incorporating code from the previous pull request submitted by jusuff. HTML5 form validation now takes precedence. Polyfills are enabled, in the event a browser does not support the validation natively. Also, AJAX validation occurs, to check if a user name and/or email address already exists.

The code appears to work correctly for me cross-browser when I tested it on Firefox and Safari. However, Chrome keeps displaying the registration form again at /register (saying that the user is already registered), instead of the expected "success" message. The issue has something to do with the behavior of "event.preventDefault()" and the function "off" on the "submit" handler, or perhaps the problem is due to the URI for the form and the "success" message being the same "/register". If anybody has any ideas about why only Chrome is misbehaving in this manner, please let me know.

Otherwise, you can test the code on Firefox and Safari to see it working.

ping @Guite @craigh @jusuff

@@ -182,14 +189,14 @@ public static function prepareJavascripts($javascripts)
$routeScript = $sm->get('router')->generate('fos_js_routing_js', array('callback' => 'fos.Router.setData'));
array_unshift($javascripts, 'web/bundles/fosjsrouting/js/router.js', $routeScript);
}
// first resolve any dependencies
// first resolve any dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@shefik
Copy link
Contributor Author

shefik commented Mar 29, 2015

@craigh I completed the updates you indicated.

$.webshims.setOptions('basePath', Zikula.Config.baseURL+'javascript/js-webshim/minified/shims/');
$.webshims.activeLang(Zikula.Config.lang);
$.webshims.polyfill(Zikula.Config.polyfillFeatures);
})(jQuery);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this version doesn't appear to be 'minified' in anyway - do we really need to two copies of the same source? how about just one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, both files are needed. It's minified, because it loads the minified shims at "javascript/js-webshim/minified/shims/". The unminified shims are loaded at "javascript/js-webshim/dev/shims".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now. I assume you are loading the minified versions in prod? non-minified in dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's using "System::isDevelopmentMode()", in the same manner in which other libraries are loaded, based on that condition.

@craigh
Copy link
Member

craigh commented Mar 29, 2015

@jusuff I would like you opinion on this before merging since it uses your code and adds to it.

@shefik
Copy link
Contributor Author

shefik commented Mar 29, 2015

@craigh The @ notion commit has been pushed.

@craigh
Copy link
Member

craigh commented Mar 29, 2015

@Guite - would also like your support before merging please.

@shefik
Copy link
Contributor Author

shefik commented Mar 29, 2015

Although this pull request specially addresses the User Registration form, it adds a new method to allow support for all of the available webshim features:

canvas
details
es5
filereader (implements FileReader, XHR2 (CORS/FormData), FormData, and an input[type="file"] picker)
forms (form validation and form features: fieldset[disabled], <input form="idref" />, placeholder...)
forms-ext (input wigets)
geolocation
matchMedia (includes matchMedia and matchMedia.addListener polyfill)
mediaelement
picture (responsive images: picture andsrcset)
promise
url (window.URL)
usermedia (navigator.getUserMedia)
sticky (position: sticky)
track (subtitles, catptions and textTrack API)
xhr2 (an alias for filereader)

The User Registration form just utilizes the polyfill for "forms". However, any of the above features can be loaded via the following examples:

{pageaddvar name='javascript' value='polyfill'}
Defaults to "forms".
{pageaddvar name='javascript' value='polyfill' features='canvas geolocation'}
PageUtil::addVar('polyfill_features', 'promise url');

Which gets added to the Zikula.Config in the head:

Zikula.Config.polyfillFeatures

@craigh
Copy link
Member

craigh commented Mar 29, 2015

@shefik - that feature requires a changelog update please.

@craigh
Copy link
Member

craigh commented Mar 29, 2015

@shefik I have this error in my javascript console (Safari)

Failed to load resource: the server responded with a status of 412 (Precondition Failed)
http://127.0.0.1/core.git/src/javascript/js-webshim/minified/shims/form-validation.js

@craigh
Copy link
Member

craigh commented Mar 29, 2015

oops - nevermind ;-)

@craigh
Copy link
Member

craigh commented Mar 29, 2015

well, I thought it was just a browser cache issue, but it continues to happen. now I get the same error but with a different link

http://127.0.0.1/core.git/src/javascript/js-webshim/minified/shims/combos/15.js

@shefik
Copy link
Contributor Author

shefik commented Mar 29, 2015

@craigh I am not experiencing that error at all in Safari.

@craigh
Copy link
Member

craigh commented Mar 29, 2015

@jusuff - do you have any ideas on the Chrome issue above? It seems to be a problem with the ajax validation.

@craigh
Copy link
Member

craigh commented Mar 29, 2015

@jusuff - @shefik says:

it really just using event.preventDefault(); and then later submit(). chrome is not liking it somehow

…ince using "submit" causes unexpected behavior in Chrome (even though other browsers such as Safari and Firefox properly handle the validation and submit.
@shefik
Copy link
Contributor Author

shefik commented Mar 30, 2015

@craigh @jusuff I fixed the issue I mentioned above, regarding Chrome. I changed the event handler to validate entries from "submit" to "blur" on the "User Name" and "Email Address" fields, since using "submit" causes unexpected behavior in Chrome (even though other browsers such as Safari and Firefox properly handle the validation and submit).

Actually, I think this change is more effective, because now other modules (such as Profile) can potentially add additional custom validations, by just attaching to the "blur" event on fields displayed from their third-party module.

@shefik
Copy link
Contributor Author

shefik commented Mar 30, 2015

@craigh CHANGELOG has been updated.

@shefik
Copy link
Contributor Author

shefik commented Mar 30, 2015

I'm done with the code now.

Guite added a commit that referenced this pull request Mar 30, 2015
Adding additional support for polyfills in Zikula core
@Guite Guite merged commit cff5d99 into zikula:1.4 Mar 30, 2015
@shefik shefik deleted the registration_form branch September 25, 2016 10:07
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0b9e6c4 on shefik:registration_form into * on zikula:1.4*.

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

4 participants