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.

Sign up for GitHub

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

Jump to bottom

Place the 3rd party JavaScript and CSS files under the third_party directory #24

Merged
merged 1 commit into from
Jun 7, 2017
Merged

Place the 3rd party JavaScript and CSS files under the third_party directory #24

merged 1 commit into from
Jun 7, 2017

Conversation

LeZhang2016
Copy link

@LeZhang2016 LeZhang2016 commented Jun 5, 2017 •
edited

fix issue #9 and #18

@LeZhang2016
Copy link
Author

@jwhui, could we just put the necessary Java script and CSS files (angular.min.js, angular-animate.min.js, angular-aria.min.js, angular-material.min.js, angular-material.min.css) into the repo as the third party lib, not all files? The Size of all files is more than 15MB.

@bukepo bukepo mentioned this pull request Jun 5, 2017
Closed
@jwhui
Copy link
Member

jwhui commented Jun 5, 2017 •
edited

I think it is fine to include a subset of files, as long as the README.md clearly lists that this only includes a subset of the files and includes any relevant LICENSE files.

jwhui
jwhui reviewed Jun 5, 2017
View reviewed changes
third_party/angular-material/README.md Outdated

## URL

https://github.com/angular/angular.js
Copy link
Member

Choose a reason for hiding this comment

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

Prefer a link to the actual release or git hash.

@bukepo
Copy link
Member

bukepo commented Jun 6, 2017

@LeZhang2016 Could you please also update the script/bootstrap and removing nodejs and bower installation? Thanks.

@LeZhang2016 LeZhang2016 force-pushed the feature-3rd-js branch 4 times, most recently from 9f71683 to f9fac18 Compare June 6, 2017 02:27
@LeZhang2016
Copy link
Author

@jwhui, @bukepo , I have updated this PR, could you help me review this PR, and let me know if you have any other comments or suggestions for the PR. Thanks a lot for your help!

bukepo
bukepo approved these changes Jun 6, 2017
View reviewed changes
Copy link
Member

@bukepo bukepo left a comment

Choose a reason for hiding this comment

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

LGTM:+1:

jwhui
jwhui reviewed Jun 6, 2017
View reviewed changes
third_party/angular-material/README.md Outdated
## Description
Material Design is a specification for a unified system of visual, motion, and interaction design that adapts across different devices. Our goal is to deliver a lean, lightweight set of AngularJS-native UI elements that implement the material design specification for use in AngularJS single-page applications (SPAs).

This repo only includes the minify version files and LICENSE file.
Copy link
Member

Choose a reason for hiding this comment

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

Change to: "Note: only the minimum set of files necessary to support the OpenThread project are included here."

third_party/angular-material/README.md Outdated
@@ -0,0 +1,22 @@
# AngularJS
Copy link
Member

Choose a reason for hiding this comment

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

Change to: "Material Design for AngularJS"

third_party/angular/README.md Outdated
## Description
Angular is a development platform for building mobile and desktop web applications using Typescript/JavaScript (JS) and other languages.

This repo only includes the minified version files and LICENSE file.
Copy link
Member

Choose a reason for hiding this comment

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

Change to: "Note: only the minimum set of files necessary to support the OpenThread project are included here."

third_party/angular/README.md Outdated
@@ -0,0 +1,22 @@
# Angular
Copy link
Member

Choose a reason for hiding this comment

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

Change to: "AngularJS"

@LeZhang2016
Copy link
Author

@jwhui, @lanyuwen, looking forward to your comments. Thanks.

@lanyuwen
Copy link
Member

lanyuwen commented Jun 7, 2017

Agree that it's fine to only include a subset of files, however, we should organize these files following the original directory structures rather than put all files flat, or it would make it hard to maintain moving forward.

@LeZhang2016
Copy link
Author

LeZhang2016 commented Jun 7, 2017 •
edited

@lanyuwen , @jwhui , @bukepo, Thanks a lot for your suggestions. I think i provided incorrect URLs about the angularJS and angular-material in README file. The previous URLs link to the source repos of the two frameworks, in which users can not obtain the built JavaScript and CSS files directly. The new URLs are from the official website, the built JS and CSS can be obtained from these URLs directly now.
AngularJS : https://code.angularjs.org/1.6.4/
AngularJS Material : https://github.com/angular/bower-material/releases/tag/v1.1.4-master-e1345ae

@jwhui jwhui requested a review from bukepo June 7, 2017 04:43
bukepo
bukepo approved these changes Jun 7, 2017
View reviewed changes
Copy link
Member

@bukepo bukepo left a comment

Choose a reason for hiding this comment

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

LGTM:+1:

@jwhui jwhui merged commit 349e3e3 into openthread:master Jun 7, 2017
@jwhui jwhui mentioned this pull request Jun 7, 2017
Closed
@gadekula1 gadekula1 mentioned this pull request Oct 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bukepo bukepo bukepo approved these changes

@jwhui jwhui jwhui approved these changes

@lanyuwen lanyuwen Awaiting requested review from lanyuwen

Assignees
No one assigned
Labels
bug P1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@LeZhang2016 @jwhui @bukepo @lanyuwen @ZhangLe2016