Date Picker using react-dates by cvializ · Pull Request #12016 · ampproject/amphtml

cvializ · 2017-11-10T21:30:07Z

Date picker effort

/to @erwinmombay could you please take a look at the build process and closure compiler pieces?
/to @aghassemi
/to @jridgewell

/cc @ericlindley-g @lswang1618

cvializ · 2017-11-10T21:33:48Z

(still looking at good ways to write tests for this build process)

aghassemi

first half

aghassemi · 2017-11-14T22:15:03Z

build-system/tasks/compile.js

      'third_party/vega/**/*.js',
      'third_party/d3/**/*.js',
      'third_party/webcomponentsjs/ShadowCSS.js',
+      'third_party/rrule/rrule.js',


let's file a security review ticket ( b/ -> Security > Consulting Services > Project Review . Assign to Gabor M) for all third-parties here.

Should I open one ticket per library, or just one ticket?

All the dependencies passed security review

aghassemi · 2017-11-14T22:22:56Z

examples/date-picker.amp.html

+  <p>Try to pick a date. This is a single-date picker</p>
+  <button on="tap:sdp.setDate(date='2017-10-22')">Set date</button>
+  <button on="tap:sdp.clear">Clear</button>
+  <amp-date


amp-date is too generic of a name IMO, could we rename to amp-date-input and amp-date-range-input if not too much work? (will be more consistent with date namespacing we have planned as well e.g. amp-date-display, amp-date-countdown, etc.. )

how about amp-date-picker and we distinguish range vs single with a type attribute?

aghassemi · 2017-11-14T22:30:06Z

examples/date-picker.amp.html

+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+</head>
+<body>
+  <h1><code>amp-date-picker</code> example</h1>


please add an experiment flag before merging and add a note here that it is under the flag.

aghassemi · 2017-11-14T22:32:22Z

examples/kayak.amp.html

@@ -0,0 +1,258 @@
+<!doctype html>


s/kayak/travel?

aghassemi · 2017-11-14T22:32:48Z

examples/kayak.amp.html

+    <span class="highlight-square"></span>
+  </template>
+</amp-date-range>
+<!-- <div [class]="!wow.show ? 'wow hidden' : 'wow'" class="wow hidden">


this section is commented out.

aghassemi · 2017-11-15T00:50:12Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+
+  /**
+   * Create an array of objects mapping dates to templates.
+   * @param {?Promise<!JsonObject|!Array<JsonObject>>} srcTemplatePromise


seems to be a void method.

It mutates this.srcTemplates_ and this.srcDefaultTemplates_, but if you'd prefer it as a pure function I would be glad to do that

aghassemi · 2017-11-15T01:00:07Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+
+  /** @return {!Promise<string>} */
+  renderInfoTemplate_() {
+    const template = scopedQuerySelector(this.element, '[info-template]');


scopedQuerySelector is an overkill for all cases I have noticed. normal queryselector should do.

aghassemi · 2017-11-15T01:05:18Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+    if (!this.infoTemplatePromise_) {
+      this.infoTemplatePromise_ = this.renderInfoTemplate_();
+    }
+    this.scanForBindings_([this.container_]);


how about other template types? would it make sense to move this to renderTemplate_?

aghassemi · 2017-11-15T01:07:20Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+      const data = opt_data || /** @type {!JsonObject} */ ({});
+      return this.templates_.renderTemplate(template, data)
+          .then(rendered => {
+            rendered.setAttribute('i-amphtml-rendered', '');


you probably don't need to set this unless you need to know which once have been rendered to clear them,

aghassemi · 2017-11-15T01:09:27Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+            rendered.setAttribute('i-amphtml-rendered', '');
+            const renderedEvent = createCustomEvent(
+                this.win_,
+                AmpEvents.DOM_UPDATE,


interesting, amp-bind is listening to this, now I don't see a need for scanForBindings_ at all. Can you test and see if scanForBindings_ is needed? (if so, why? looks like bind should be re-scanning on dom mutations anyway)

It looks like scanForBindings is not needed, I was dispatching the event on the wrong element. It was incorrectly dispatched on the template element, which of course doesn't contain any elements with bindings.

Date picker with dist build working

cvializ · 2017-12-06T19:03:13Z

@aghassemi This should be ready for further review now!

cvializ · 2017-12-06T22:12:44Z

build-system/dep-check-config.js

  },
+  {
+    filesMatching: '**/*.js',
+    mustNotDependOn: 'src/module.js',


Do you think this is helpful, or unnecessary? I thought it might make sense to explicitly opt-in to this pattern.

I like it, good thinking.

aghassemi

Amazing PR Carlos.
Few small requests and some suggestions.

For testing (can be after this PR), I suggest unit tests for the utils and util-like functions and then e-2-e integration tests for the rest. The integration tests can be simple to write and can cover a huge portion of the code.

aghassemi · 2017-12-08T19:25:19Z

css/amp.css

 }
+
+amp-date,
+amp-date-range {


element was renamed, so these need updating.

Overall I really like the idea of forcing container and having these to make sure pre-attach and post-attach are the same. Maybe also add a comment here explaining why these are here.

For default style changes, I hope we can make most of the changes (for the input part) here instead of date-picker.css so we don't end up duplicating them.

aghassemi · 2017-12-08T19:27:07Z

build-system/amp.extern.js

      error: (RTC_ERROR_ENUM|undefined)}} */
 var rtcResponseDef;
+
+/** @type {!function(string):?} */


nit: short summary comment on why we have this

aghassemi · 2017-12-08T19:28:29Z

build-system/dep-check-config.js

  },
+  {
+    filesMatching: '**/*.js',
+    mustNotDependOn: 'src/module.js',


I like it, good thinking.

aghassemi · 2017-12-08T19:29:19Z

build-system/scope-require.js

@@ -0,0 +1,135 @@
+/**
+ * Copyright 2017 The AMP HTML Authors. All Rights Reserved.


/cc @erwinmombay @rsimha-amp for the build setup.

aghassemi · 2017-12-08T19:38:49Z

gulpfile.js

+      'third_party/react-dates/bundle.js',
+    ]);
+  } else if (srcFilename === 'amp-viz-vega.js') {
+    bundle = concatFilesToString([


aghassemi · 2017-12-08T20:03:56Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+          .filter(t => t.dates)
+          .map(t => ({
+            dates: new DatesList(t.dates),
+            template: this.ampdoc_.getRootNode().querySelector(


I assume templates can't be outside of <amp-date-picker> (mostly due to validator restrictions)., unless I am mistaken we should be able to use this.element for the query.

I think template-impl.js supports templates outside the parent element via the template="" attribute, which queries for the id in the whole document, so I think it shouldn't be too problematic for the validator to do it for the date-picker too.

amphtml/src/service/template-impl.js

Lines 226 to 233 in 4ab32dc

maybeFindTemplate_(parent) {

const templateId = parent.getAttribute('template');

if (templateId) {

return parent.ownerDocument.getElementById(templateId);

} else {

return childElementByTag(parent, 'template');

}

}

aghassemi · 2017-12-08T20:06:47Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+
+      const defaultTemplate = templates
+          .filter(t => t.dates == null)
+          .map(t => this.ampdoc_.getElementById(t.id))[0];


ditto for getElementById, scoping to this element via querySelector.

(although ids have to be unique, we don't restrict it and I have seen bugs in AMP pages caused by duplicated ids, this sort of scoping can help be more forgiving of such user mistakes)

aghassemi · 2017-12-08T20:14:41Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+            return rendered./*REVIEW*/outerHTML;
+          });
+    } else {
+      return Promise.resolve(opt_fallback || '');


might be worth calling sanitizeFormattingHtml from sanitizer.js on fallback anyway for additional protection.

aghassemi · 2017-12-08T20:15:26Z

extensions/amp-date-picker/0.1/amp-date-picker.js

+  }
+}
+
+AMP.extension(TAG, '0.1', AMP => {


nicely organized extension! awesome job.

aghassemi · 2017-12-08T20:18:47Z

src/service/template-impl.js

   * Find a specified template inside the parent. Fail if the template is
   * not present.
   * @param {!Element} parent
+   * @param {string=} opt_queryString


nit: Maybe querySelector?, queryString makes me think of Urls

rsimha · 2017-12-13T18:21:46Z

@cvializ @aghassemi After this PR was landed, I'm seeing the following warnings when I run yarn on linux. Could it be that some implicit dependencies weren't added to package.json?

warning "react-dates@13.0.6" has unmet peer dependency "react@>=0.14".
warning "react-dates@13.0.6" has unmet peer dependency "react-dom@>=0.14".
warning "airbnb-prop-types@2.8.1" has unmet peer dependency "react@^0.14 || ^15.0.0 || ^16.0.0-alpha".
warning "react-with-styles@2.3.0" has unmet peer dependency "react@>=0.14".
warning "react-with-styles@2.3.0" has unmet peer dependency "react-with-direction@^1.1.0".
warning "react-with-direction@1.1.0" has unmet peer dependency "react@^0.14 || ^15 || ^16".
warning "react-with-direction@1.1.0" has unmet peer dependency "react-dom@^0.14 || ^15 || ^16".

cvializ · 2017-12-13T19:39:33Z

Ah yeah, thanks for looking out! I was aware of these warnings, and I looked for a way to remove just those warnings, but I didn't find a good way. This PR used preact instead of react, but I am about to submit another PR that will use react since the license issues were resolved and there are some bugs with how preact and react-dates interact. That new PR should resolve these peer dependency errors.

* Initial wip commit for react-dates date picker Date picker with dist build working * Type lint and type checking mostly working * Fix presubmit errors * Update whitelist * First set of fixes for review feedback * Remove scanForBindings_ * Add experiment * Add externalRequire helper. Remove global library references. Types. * Rename. Remove unnecessary node_modules and changes * Remove need for additional globals like require. * Fix presubmit check * Add experiment flag to example * Address review feedback. Phrasing. * whitelist sanitizer

cvializ requested review from aghassemi, erwinmombay and jridgewell November 10, 2017 21:30

googlebot added the cla: yes label Nov 10, 2017

cvializ force-pushed the feature/date-picker branch from 053b242 to ccabf0f Compare November 10, 2017 23:04

cvializ mentioned this pull request Nov 13, 2017

Weekly Status 2017-11-13 #12026

Closed

ampprojectbot added this to the Pending Triage milestone Nov 13, 2017

aghassemi suggested changes Nov 15, 2017

View reviewed changes

cvializ force-pushed the feature/date-picker branch from b8f02dd to 64e140d Compare November 30, 2017 22:41

Carlos Vializ added 8 commits December 1, 2017 14:31

Initial wip commit for react-dates date picker

3616366

Date picker with dist build working

Type lint and type checking mostly working

df591dc

Fix presubmit errors

506baef

Update whitelist

650a82e

First set of fixes for review feedback

2be898e

Remove scanForBindings_

2619e36

Add experiment

a58066b

Add externalRequire helper. Remove global library references. Types.

d7421b2

cvializ force-pushed the feature/date-picker branch 2 times, most recently from adb138e to dd12a10 Compare December 1, 2017 23:15

Rename. Remove unnecessary node_modules and changes

dee99e8

cvializ force-pushed the feature/date-picker branch from dd12a10 to dee99e8 Compare December 1, 2017 23:27

Remove need for additional globals like require.

7c41aa2

cvializ force-pushed the feature/date-picker branch from 9efd70d to 7c41aa2 Compare December 6, 2017 18:50

Merge branch 'master' into feature/date-picker

fa5da8e

Fix presubmit check

0427844

cvializ commented Dec 6, 2017

View reviewed changes

Add experiment flag to example

d776060

aghassemi suggested changes Dec 8, 2017

View reviewed changes

Carlos Vializ added 2 commits December 8, 2017 17:23

Address review feedback. Phrasing.

fe4408e

whitelist sanitizer

f43b4a1

aghassemi mentioned this pull request Dec 11, 2017

Status Update 2017-12-11 #12402

Closed

aghassemi approved these changes Dec 11, 2017

View reviewed changes

cvializ merged commit 768fad0 into ampproject:master Dec 11, 2017

ampprojectbot added the PR use: In Beta / Experimental label Dec 13, 2017

rsimha mentioned this pull request Dec 21, 2017

Move react-dates, etc. from devDependencies to dependencies #12571

Merged

ericlindley-g mentioned this pull request Apr 19, 2018

[Master feature] Date picker #14744

Closed

rsimha mentioned this pull request Dec 27, 2019

Restore RRule support in amp-date-picker #26154

Closed

		@@ -0,0 +1,135 @@
		/**
		* Copyright 2017 The AMP HTML Authors. All Rights Reserved.

	maybeFindTemplate_(parent) {
	const templateId = parent.getAttribute('template');
	if (templateId) {
	return parent.ownerDocument.getElementById(templateId);
	} else {
	return childElementByTag(parent, 'template');
	}
	}

Conversation

cvializ commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvializ commented Nov 10, 2017

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvializ commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvializ Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsimha commented Dec 13, 2017

Uh oh!

cvializ commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

cvializ commented Nov 10, 2017 •

edited

Loading

cvializ Dec 8, 2017 •

edited

Loading

cvializ commented Dec 13, 2017 •

edited

Loading