Skip to content

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

Merged
rsimha merged 1 commit intoampproject:masterfrom
rsimha:2017-12-21-ReactDates
Jan 4, 2018
Merged

Move react-dates, etc. from devDependencies to dependencies#12571
rsimha merged 1 commit intoampproject:masterfrom
rsimha:2017-12-21-ReactDates

Conversation

@rsimha
Copy link
Copy Markdown
Contributor

@rsimha rsimha commented Dec 21, 2017

This is a follow up to the discussion in #12472 (review)

In #12016 and #12456, @cvializ added a bunch of packages to devDependencies while writing a new date picker component for the AMP runtime. It's possible that some of those should actually go into dependencies. See these diffs for the full list of packages that were added.

https://github.com/ampproject/amphtml/pull/12016/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2.
https://github.com/ampproject/amphtml/pull/12456/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

This PR moves the packages required by the date picker from devDependencies to dependencies and reverts them to their originally added versions.

Follows up #11368, #12016, #12456, and #12472
Partial fix for #12181

@rsimha rsimha self-assigned this Dec 21, 2017
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Dec 21, 2017

/to @erwinmombay @cvializ

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Dec 21, 2017

It's possible that the only package that needs to move to dependencies is react-dates, since the only import from node_modules in amp-date-picker is here: https://github.com/ampproject/amphtml/blob/master/extensions/amp-date-picker/0.1/amp-date-picker.css

Will let @cvializ comment further.

@rsimha rsimha changed the title Move react-dates, etc. to dependencies and update instructions Move react-dates, etc. from devDependencies to dependencies Dec 21, 2017
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Dec 22, 2017

@cvializ Ping.

@erwinmombay
Copy link
Copy Markdown
Member

erwinmombay commented Dec 22, 2017

@rsimha-amp @cvializ might be on vacation, if i remember correctly what he told me

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Dec 22, 2017

@erwinmombay Calendar didn't seem to suggest that, but you may be right. Since there's no upcoming release soon, we can leave this until after the holidays :)

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jan 2, 2018

Rebased. Bumping this.

Copy link
Copy Markdown
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

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

I don't see any issues with that, LGTM

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Jan 4, 2018

@cvializ Thanks for checking.

@rsimha rsimha merged commit 5b22c0d into ampproject:master Jan 4, 2018
@rsimha rsimha deleted the 2017-12-21-ReactDates branch January 4, 2018 17:45
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
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.

4 participants