Skip to content
This repository has been archived by the owner on Sep 7, 2022. It is now read-only.

Add differentiated project templates #2

Merged
merged 14 commits into from
Feb 17, 2017
Merged

Add differentiated project templates #2

merged 14 commits into from
Feb 17, 2017

Conversation

FredKSchott
Copy link
Contributor

@FredKSchott FredKSchott commented Feb 14, 2017

  • Converts polymer-1-app to use differentiated, polymer v1-specific patterns
  • Adds polymer-2-app with differentiated, polymer v2-specific patterns
  • Paves the way for polymer-1-hybrid-app and polymer-2-hybrid-app which will be simple combinations of these two projects. (Future PR)

It was too difficult to coordinate changes across two different PRs, so my work on both projects is here. But they are separated by commit, if you'd prefer to review that way.

/cc @justinfagnani @rictic @usergenic

Copy link

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Looks good to my initial review. We should be able to publish this as @polymer/tools-sample-projects, add a dev dependency on it in CLI and run over these scenarios in the CLI's integration tests.

We still might want to do a manual test to ensure that everything looks right (the styles are correct, etc), but it would go a long way.

@@ -1,6 +1,6 @@
<!--
@license
Copyright (c) 2016 The Polymer Project Authors. All rights reserved.
Copyright (c) 2017 The Polymer Project Authors. All rights reserved.
Copy link

Choose a reason for hiding this comment

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

Here and elsewhere: general guidance is not to update copyright date for existing files. If these files were actually created in 2017 then this is just a correction and probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are corrections in polymer-1-app and new additions in polymer-2-app. This repo was started in 2017 but this was done for completeness more than anything. Happy to back out if we prefer to leave it as is.

@@ -22,7 +22,7 @@
<link rel="import" href="../bower_components/paper-icon-button/paper-icon-button.html">
<link rel="import" href="my-icons.html">

<dom-module id="my-app">
<dom-module name="my-app">
<template>
<style>
Copy link

Choose a reason for hiding this comment

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

<style> can be outside <template> (but still inside dom-module) for 1.0 elements, if you're looking to do idiosyncratic 1.0 things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added name-view already as an example of name vs. id and some other patterns like <style> outside of <template>. I'll leave them there to represent 1.0 things and back out this change since it is no longer strictly needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rictic that's been deprecated since 1.1 though.

@@ -0,0 +1,97 @@
# Polymer App Toolbox - Starter Kit
Copy link

Choose a reason for hiding this comment

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

Does this readme apply here? Recommend dropping or updating it.

@justinfagnani
Copy link
Contributor

Looks good, but I think there's a lot of things that could be trimmed to leave the more salient bits. Let's just get this in now though.

@@ -126,12 +130,12 @@

_pageChanged: function(page) {
// Load page import on demand. Show 404 page if fails
var resolvedPageUrl = this.resolveUrl('my-' + page + '.html');
var resolvedPageUrl = this.resolveUrl('views/' + page + '.html');
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use lazy imports in one of the 1.0 scenarios

If deploying in a folder replace href="/" with the full path to your site.
Such as: href=="http://polymerelements.github.io/polymer-starter-kit"
-->
Oops you hit a 404. <a href="/">Head back to home.</a>
<div class="card">
Oops you hit a 404. <a href="/">Head back to home.</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this view testing?

<link rel="import" href="../../bower_components/polymer/polymer.html">
<link rel="import" href="../shared-styles.html">

<!-- element template -->
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove these comments

<div>
<content></content>
<br />
<div>There are <strong>[[name]]</strong> childNode elements total.</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this element testing?

}
</style>

<!--
<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this. In general I'd remove a lot of PSK guidance comments and they're probably noise in the way of us seeing what's relevant.

}
</script>

<script src="/bower_components/webcomponentsjs/webcomponents-lite.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

use webcomponents-loader.js

}
}

window.customElements.define(MyApp.is, MyApp);
Copy link
Contributor

Choose a reason for hiding this comment

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

ooh, interesting, we need to account for not having a string literal

@FredKSchott
Copy link
Contributor Author

@justinfagnani agreed, there's still some PSK-related stuff in the templates. This is nice because it gives us a way to serve and navigate between the views to make sure they're all displaying properly, but it does mean there's more than we need from a tooling perspective.

@FredKSchott FredKSchott merged commit 17704c4 into master Feb 17, 2017
@FredKSchott FredKSchott deleted the add-projects branch February 17, 2017 19:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants