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

Polyfill reportValidation calls and bubble UI for webkit. #3734

Merged
merged 7 commits into from Jun 28, 2016

Conversation

mkhatib
Copy link
Contributor

@mkhatib mkhatib commented Jun 23, 2016

Test this out on any webkit-based browser at the demo here.

This polyfill the bubble UI for validation reporting in webkit browsers. This matches browsers implementation of one invalid bubble at a time, focusing the invalid input, hide bubble on blur and continuous revalidation as the user types in an active invalid reported input.

  • Figure out why the example is working on Safari Desktop and iOS simulator but not on real iOS - This was due to the demo page being served on http instead of https.
  • Add and fix tests

ITI: #3343

@mkhatib
Copy link
Contributor Author

mkhatib commented Jun 23, 2016

Test this out on any webkit-based browser at the demo here.

@mkhatib
Copy link
Contributor Author

mkhatib commented Jun 23, 2016

Want to get some feedback on this before writing tests. Let me know if this looks good!

@dvoytenko
Copy link
Contributor

@mkhatib Can't get demo to work.

@cramforce cramforce removed their assignment Jun 23, 2016


.-amp-validation-bubble {
background-color: white;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a 3-space indent?

Copy link
Contributor Author

@mkhatib mkhatib Jun 23, 2016

Choose a reason for hiding this comment

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

4 Just noticed my editor has 4-space indent for css, will fix that to 2 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mkhatib
Copy link
Contributor Author

mkhatib commented Jun 24, 2016

Updated! Thanks for all the great comments! PTAL.

e.preventDefault();
this.vsync_.run({mutate: reportValidity}, {form: this.form_});
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to use the same {measure: undefined, mutate: reportValidity} hidden class. 😞

I'll file an issue to add state to #measure and #mutate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. This is for type matching of the Task def, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -111,7 +111,10 @@ export class AmpForm {
if (shouldValidate &&
this.form_.checkValidity && !this.form_.checkValidity()) {
e.preventDefault();
this.vsync_.run({mutate: reportValidity}, {form: this.form_});
this.vsync_.run({
measure: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a TODO #3776, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* @private
*/
function measureTargetElement(state) {
state.targetRect = state.viewport.getLayoutRect(state.targetElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it ever calculate with display: none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

targetElement is not display:none that is the bubbleElement.

@mkhatib mkhatib changed the title WIP: Polyfill reportValidation calls and bubble UI for webkit. Polyfill reportValidation calls and bubble UI for webkit. Jun 27, 2016
@mkhatib
Copy link
Contributor Author

mkhatib commented Jun 27, 2016

Added tests and addressed all comments. PTAL.

@jridgewell
Copy link
Contributor

LGTM.

@mkhatib mkhatib added LGTM and removed NEEDS REVIEW labels Jun 28, 2016
@mkhatib mkhatib merged commit ec96f9c into ampproject:master Jun 28, 2016
@mkhatib mkhatib deleted the tooltip-component branch June 28, 2016 00:10
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

5 participants