Skip to content

s/preact/react/. Strip non-prod code from react-dates bundle. Import normally#12456

Merged
cvializ merged 3 commits intoampproject:masterfrom
cvializ:feature/tweak-dp
Dec 14, 2017
Merged

s/preact/react/. Strip non-prod code from react-dates bundle. Import normally#12456
cvializ merged 3 commits intoampproject:masterfrom
cvializ:feature/tweak-dp

Conversation

@cvializ
Copy link
Copy Markdown
Contributor

@cvializ cvializ commented Dec 13, 2017

This decreases the amp-date-picker dist binary size and uses more normal import mechanisms.

Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

package.json and yarn.lock changes LGTM!

* @return {?Promise<!JsonObject|!Array<JsonObject>>}
*/
fetchSrcTemplates_() {
if (assertHttpsUrl(this.element.getAttribute('src'), this.element)) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

assertHttpsUrl is already called in fetchedBatchedJsonFor, so it was redundant here.

* @param {?string} focusedInput
*/
onFocusChange(focusedInput) {
// TODO(cvializ): there is a bug in the react-dates library that
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This bug only happened when using preact-compat with react-dates, so this workaround code isn't needed any more.

@dreamofabear
Copy link
Copy Markdown

decreases the amp-date-picker dist binary size

Surprising. I thought preact's value prop was significantly reduced size?

@cvializ
Copy link
Copy Markdown
Contributor Author

cvializ commented Dec 13, 2017

No that is still true, but the way the PR decreases the size is using envify and uglify to strip blocks where if (process.env.NODE_ENV === 'development') {}. The easiest way to fix the onFocusChange bug in DateRangePicker was to use react. I will check again in the near-future to see if the size difference is enough of an improvement to justify trying to fix the onFocusChange bug.

@import '../../../node_modules/react-dates/lib/css/_datepicker.css';

.DateInput_input {
font-size: 100%;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Prevents iOS from zooming in when tapping on the hidden input. The default was 11px. Anything less than 16px triggers auto-zooming on focus.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wow, good to know!

@import '../../../node_modules/react-dates/lib/css/_datepicker.css';

.DateInput_input {
font-size: 100%;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wow, good to know!

@cvializ cvializ merged commit 8a0e33f into ampproject:master Dec 14, 2017
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
…normally (ampproject#12456)

* s/preact/react/. Strip non-prod code from react-dates bundle. Import normally

* Fix deps

* yarn.lock?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants