-
Notifications
You must be signed in to change notification settings - Fork 660
Conversation
|
license
xxx xxxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added changes at #1097.
Polymer 3 project update
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
LICENSE
Outdated
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER | ||
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, | ||
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file - canonical license file is already referenced at the top of source files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files allow us to have the GitHub license preview:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack on the GitHub license preview, but this is not something we're broadly adopting across our repos at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we leaving the license
field in package.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's fine (needed to suppress npm warning).
polymer.json
Outdated
@@ -2,7 +2,8 @@ | |||
"entrypoint": "index.html", | |||
"shell": "src/my-app.js", | |||
"sources": [ | |||
"images/**/*" | |||
"images/**/*", | |||
"src/**/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I've been removing them in other 3.0 projects since it didn't seem to do anything (build discovers the tree through import statements).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The truth is that I'm not sure.
Ping to @aomarks @justinfagnani @FredKSchott ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usergenic might also know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the source file is imported somewhere with a string literal identifier (i.e. not by concatenation or variable reference) then you shouldn't need them here. However this is good if your source filenames can not be statically resolved by build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to revert this then - it doesn't seem to be affecting anything yet. Also, I talked w/ @usergenic off of GitHub and he said that shell
is one of the roots of this graph and dynamic imports are considered when finding other sources. Given that src/my-app.js
eventually gets to all of the files in src
through both regular and dynamic imports, it should be sufficient to get the whole graph. This should be easy to add back later if we find it's better.
… the top of files.
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@@ -79,14 +79,14 @@ Run `polymer help build` for the full list of available options and optimization | |||
|
|||
This command serves your app. Replace `build-folder-name` with the folder name of the build you want to serve. | |||
|
|||
polymer serve build/build-folder-name/ | |||
npm start build/build-folder-name/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this true? i.e. have you tested this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdonrd added these as npm scripts in package.json
, they use the cli there, and I think this means that you don't have to install the cli globally to run through these instructions anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bicknellr exactly! Use the Polymer CLI installed in the project.
It helps to avoid install the Polymer CLI globally. And more now that it is in alpha.
Also the users can use the common npm start
, npm test
, etc scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
<!-- Load your application shell --> | ||
<script type="module" src="./src/my-app.js"></script> | ||
<script type="module" src="src/my-app.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are the relative paths wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set it like this and use the base tag to support the PRPL Server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this change makes a difference since it's still a relative URL that uses the base tag. No action needed.
src/my-app.js
Outdated
subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt | ||
*/ | ||
* @license | ||
* Copyright (c) 2018 The Polymer Project Authors. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blehhhh pinging @azakus to double check we should be bumping this year (i guess it's technically a new file?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Substantial edits have been made, so updating is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only new new files need a new date AFAIK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the dates back.
setPassiveTouchGestures(true); | ||
|
||
setRootPath(Polymer.rootPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polymer
doesn't seem to be getting imported from anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's set as a global in index.html
. I'm going to skip doing anything about that in this PR but I think these might make sense to move out to index.html
too? I've opened a branch (global-settings
) to look at this.
|
||
### Build | ||
|
||
The `polymer build` command builds your Polymer application for production, using build configuration options provided by the command line or in your project's `polymer.json` file. | ||
The `npm run build` command builds your Polymer application for production, using build configuration options provided by the command line or in your project's `polymer.json` file. | ||
|
||
You can configure your `polymer.json` file to create multiple builds. This is necessary if you will be serving different builds optimized for different browsers. You can define your own named builds, or use presets. See the documentation on [building your project for production](https://www.polymer-project.org/2.0/toolbox/build-for-production) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should find the new link for https://www.polymer-project.org/2.0/toolbox/build-for-production.
src/my-app.js
Outdated
</iron-pages> | ||
</app-header-layout> | ||
</app-drawer-layout> | ||
`; | ||
} | ||
|
||
static get is() { return 'my-app'; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the is
getter is necessary anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
FWIW, I think googlebot was worried that my merge commit which has my personal email attached to it (thanks to GitHub not letting me choose which to use on which repos) is somebody new. |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Chill out googlebot, it's still just me. |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
bower.json
.bower.json
, which previously linked to the license text.polymer test
; possibly related topolymer serve
starting in 'component' mode?./node_modules/dep
vs../dep
)