Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Html helper #2412

Merged
merged 9 commits into from
Feb 1, 2018
Merged

Html helper #2412

merged 9 commits into from
Feb 1, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 7, 2017

@ghost ghost requested a review from arthurevans December 7, 2017 04:35
@ghost
Copy link
Author

ghost commented Dec 7, 2017

Fixes #2407

@ghost ghost requested a review from justinfagnani December 7, 2017 04:56
@justinfagnani
Copy link
Contributor

LGTM!

My only small nit is not with this change, but how we talk about properties like template and properties. We always call them the "static X getter", but really, a getter is just one way to implement the property.

You could also just set the property on the constructor:

class MyElement {}
MyElement.template = html`...`;

Or if you using a compiler, and in the near future in native JS, declare a static property:

class MyElement {
  static template = html`...`;
}

@ghost
Copy link
Author

ghost commented Dec 8, 2017 via email

@justinfagnani
Copy link
Contributor

Oh yeah, this looks good to me as-is, I just wanted to mention that nit since it's been on my mind for a while.

@ghost
Copy link
Author

ghost commented Dec 8, 2017

Sweet, thanks Justin. I'll get @arthurevans to give it a look, then merge if he's cool with it.

- [Specify a template using the `<dom-module>` element](#dommodule). This allows you to specify the
template entirely in markup, which is most efficient for an element defined in an HTML
import.
- [Assign a string template to an HTMLTemplateElement object, and return the

Choose a reason for hiding this comment

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

I like the new links.

I think the description of this one focuses too much on the implementation. The gist here is that you can define a template getter that returns the template.

Except that @justinfagnani raised a concern about the wording of that. Um, so,...

"Add a template property on the constructor that returns the template?"

"Provide a template by defining a template property on the constructor"?

}
}
customElements.define('my-element', MyElement);
```

When using a string template, the element doesn't need to provide an `is` getter (however, the tag
name still needs to be passed as the first argument to `customElements.define`).
Alternatively, declare a variable called `html` to hold the `Polymer.html` object, and

Choose a reason for hiding this comment

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

I feel like this should be listed goal-first. It's a lot of text to read through to figure out that you'd want to do this to enable syntax highlighting.

As an alternative to specifying the element's template in markup, you can specify a string template
by creating a static `template` getter that returns a string.
As an alternative to specifying the element's template in markup, you can create a static
`template` getter that returns an HTMLTemplateElement.

Choose a reason for hiding this comment

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

HTMLTemplateElement should be in code format.

The magic Polymer.html should be introduced, like, "Polymer provides an html function that can be applied to a template literal to produce an HTMLTemplateElement." Or something @justinfagnani ?

Unfortunately, ECMAScript kind of failed when it comes to terminology here. foo is a template literal. htmlfoo is a tagged template literal. What exactly the html function is in this syntax is not specified, AFAICT, in the ECMAScript spec. It's... the function that you tag the literal with. Or something.

Sometimes I hate engineers. Just sayin'.

Anyway, point is, I'm basically just specifying a string, but presto change-o, I return a full-blown template element.

@arthurevans
Copy link

Few comments in today. I have to run, will finish up tomorrow. I think we should iterate a little more here...

@ghost
Copy link
Author

ghost commented Dec 8, 2017

@arthurevans Great points, thank you. Incorporated all. Let's iterate until it's ready~

- If you're doing anything expensive, like copying or modifying an existing template,
you should memoize the modified template so you don't have to regenerate it when the
getter is called.
#### Inherit a base class template with no modifications {#nomods}

Choose a reason for hiding this comment

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

These examples should use dom-module, since it's the default mechanism for Polymer 2.x.

Copy link
Author

Choose a reason for hiding this comment

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

Converted back to dom-module examples - a couple of issues with the following

http://plnkr.co/edit/f5utiX?p=preview
http://plnkr.co/edit/YQ5t5I?p=preview

@arthurevans @justinfagnani

Choose a reason for hiding this comment

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

I meant that specifically for the "inherit a base class with no modifications."

The ${expr} format is specific to template literals (those things in `backticks`) and won't work in <template> elements... despite the names, they're rather different creatures.

The extension examples you added with the html function and template literals are much clearer than the previous ones that require manipulating template elements in JS. And for that reason I think we might want to keep them in. However, they'll only work in 2.4+, so we might want to leave in at least one of the previous examples.

I had some more specific comments on this stuff inline.

As an alternative to specifying the element's template in markup, you can specify a string template
by creating a static `template` getter that returns a string.
As an alternative to specifying the element's template in markup, you can define a static `template`
property. The `template` property must be an `HTMLTemplateElement`.
Copy link

@arthurevans arthurevans Dec 8, 2017

Choose a reason for hiding this comment

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

This section is correct for 3.x, but for 2.x we support returning a string, as well.

Maybe in place of the second sentence, add something to the next para, like,

In Polymer 2.x, the template can be supplied as an HTMLTemplateElement instance or a string. Support for returning a string template is being discontinued in Polymer 3.x. Starting in Polymer 2.4 <<>>, Polymer provides ... This is the recommended way of defining a string template in Polymer 2.4 and later.

}
}
customElements.define('my-element', MyElement);
```

When using a string template, the element doesn't need to provide an `is` getter (however, the tag
name still needs to be passed as the first argument to `customElements.define`).
To enable HTML code highlighting in text editors with lit-html code highlighting functionality, you

Choose a reason for hiding this comment

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

"with lit-html code highlighting functionality" => I gather this isn't strictly true; some just have support for this built-in according to @justinfagnani ... Also, somewhat confusing to mention this in this context, because the html already looks kind of like lit-html, and we don't want people to confuse the two.

"some text editors" is kind of lame.

the full explanation is probably a bit wordy, would probably be something like,
"Some text editors support HTML code highlighting in template literals tagged with a function named html." Again, @justinfagnani to confirm perhaps?

When using a string template, the element doesn't need to provide an `is` getter (however, the tag
name still needs to be passed as the first argument to `customElements.define`).
To enable HTML code highlighting in text editors with lit-html code highlighting functionality, you
can declare a variable called `html` to hold the `Polymer.html` object, and return it from a `template` getter:

Choose a reason for hiding this comment

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

Two details: Polymer.html is a function, and html here is a constant.

"And return it from the template getter..." sounds like you're returning the function. Use it in the template getter, perhaps?

```

Child class definition { .caption }
```js

Choose a reason for hiding this comment

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

Arguably this is simpler than using the existing template manipulation code, but it involves some magic that should probably be explained.

  1. The ${expression} inside a template literal interpolates the value of an expression. (This is basic ES6, and maybe it doesn't need to be spelled out, but if so, we should at least link to the MDN description of template literals at the first reference (in the string template section), because we're using a whole bunch of their features here and not everyone is familiar with them yet.)
  2. Because the getter is static, super.template accesses the template property on the superclass constructor.
  3. The Polymer.html function checks each expression value. If the value is a string, the string is interpolated directly. If the expression is an HTMLTemplateElement, the innerHTML of the template is interpolated. This makes it very simple to combine existing templates.

Choose a reason for hiding this comment

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

And this is a lot of words, and maybe should be pushed up to some concepts section somewhere, but I think it's important because we're using a lot of ES6-fu here that seems completely transparent to the @justinfagnani s of the world...

```

[See a working example in Plunker](http://plnkr.co/edit/f5utiX?p=preview).

#### Insert content from a child class into placeholders supplied by a base class {#insertcontent}
Copy link

@arthurevans arthurevans Dec 8, 2017

Choose a reason for hiding this comment

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

I'm not sure this is the correct title for this use case... It seems like the primary use case here is the superclass use case: that is, "provide overrideable sub-templates." Or something. Replaceable sub-templates? Allow easy template extension with modular templates?

@justinfagnani @kevinpschaaf @graynorton @sorvell Anyone want to name this pattern? :D

Just seems to me that the important part is, a superclass providing a focused way to extend its template with content. They could be placeholders, or not—for example, they could be a valid part of the superclass template that a subclass may or may not replace.

Choose a reason for hiding this comment

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

Also: We may want to leave in one or both of the old-school template extension examples, since they're still valid and html`${foo}` will only work in 2.4 and later.

Copy link
Member

Choose a reason for hiding this comment

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

  • "Template extension points"
  • "Overridable sub-templates"

I'd probably go with the first...?

@ghost
Copy link
Author

ghost commented Dec 12, 2017

@arthurevans @justinfagnani Revised the page based on feedback. Please have another look. https://html-helper-dot-polymer-project.appspot.com/2.0/docs/devguide/dom-template is updated with the revised version.

@justinfagnani
Copy link
Contributor

@katejeffreys the one question I have is whether or not it's confusing to mention plain string templates at all. The "Specify a string template in a static template getter" example will be deprecated in the next release. Maybe remove that example and add a note that says something like "Note: It used to be possible to specify a plain string template as a static property on your element class, but this feature was deprecated in 2.? and will be removed in Polymer 3.0"

Copy link

@arthurevans arthurevans left a comment

Choose a reason for hiding this comment

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

Few more comments on the new sections.

(for example, in an ES6 module).
- Retrieve or generate your own template element. This allows you to define how the template is
retrieved or constructed, and can be used to modify a superclass template.
- [Specify a template using the `<dom-module>` element](#dommodule). This allows you to specify
Copy link

@arthurevans arthurevans Dec 13, 2017

Choose a reason for hiding this comment

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

Looking over this (and especially the section headings that follow) I think it'd be good to simplify the links/titles here:

  • Use the <dom-module> element.
  • Define a template property on the constructor.

Using these titles (or titles like them) the headings are more parallel and the TOC is easier to read, I think.

Copy link
Author

@ghost ghost Jan 8, 2018

Choose a reason for hiding this comment

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

  • update headings and links to make parallel & simpler

### Provide a template by defining a template property on the constructor {#templateobject}

As an alternative to specifying the element's template in markup, you can define a static
`template` property.
Copy link

@arthurevans arthurevans Dec 13, 2017

Choose a reason for hiding this comment

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

static template property => template property on the constructor.

ES6 doesn't currently support static properties per se, only static getters. If someone's using a transpiler for future ES features like static properties, they should know how to take advantage of this.

Theoretically, as @justinfagnani said, they could also declare the template by adding a property directly on the constructor, like:

class FooBar extends Polymer.Element { ... }
FooBar.template = Polymer.html`something` // or whatever...

I don't think we should show an example like the one I just showed in the doc, because it adds another permutation without providing any particular benefit.

Copy link
Author

@ghost ghost Jan 8, 2018

Choose a reason for hiding this comment

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

  • static template property => template property on the constructor.

`template` property.

One way to do this is to specify a string template by creating a static template getter that
returns a template literal.

Choose a reason for hiding this comment

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

This isn't quite right. It's returning an instance of HTMLTemplateElement.

It looks kind of like you're returning a template literal when you type this:

return Polymer.html`<div>[[hello]]</div>`;

However, it's actually what the spec calls a "tagged template literal," where the "tag" (Polymer.html) is a function, and what you're actually returning is the result of calling the Polymer.html function on the template literal.

Choose a reason for hiding this comment

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

Oh, wait. You're showing a plain string template first.

I think the outline of this section is kind of confusing. For example, you talk about embedded expressions below, then introduce an example that doesn't have any.

You could start off by saying something like:

The template getter can return either a string or an instance of HTMLTemplateElement. Support for the string return value is deprecated in Polymer 2.4, and will be removed in 3.0. Polymer 2.4 provides a helper function to generate an HTMLTemplateElement instance from a template literal.

And then maybe include two subsections: "Using the Polymer.html helper (2.4+)" and "Returning a string template (all 2.x versions)." Does that work?

@justinfagnani the problem with omitting the string version entirely is that, if the past is any indication, a lot of our users are going to be on pre-2.4 releases for a while.

Copy link
Author

@ghost ghost Jan 8, 2018

Choose a reason for hiding this comment

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

  • clarify intro to this section
  • make sure terminology is correct
  • subsection: 2.4+
  • subsection: 2.x all

provide its own DOM template (using either a `<dom-module>` or a string template), Polymer uses the
same template as the superclass, if any.
provide its own DOM template (using either a `<dom-module>` or a static template object), Polymer
uses the same template as the superclass, if any.

Choose a reason for hiding this comment

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

I think this list of items needs some introduction, like:

The Polymer.html helper introduced in Polymer 2.4 makes template extension a little simpler, so this section shows examples with Polymer.html (usable on 2.4 and later) and without (usable on all 2.x versions).

Or... something.

Copy link
Author

@ghost ghost Jan 8, 2018

Choose a reason for hiding this comment

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

  • intro to list

@arthurevans
Copy link

Ping @katejeffreys on the updates here. As soon as we have those updates we can merge this for the 2.4 release and cut a 2.x branch.

@ghost
Copy link
Author

ghost commented Jan 6, 2018

Thanks for the reminder! Will do.

@justinfagnani
Copy link
Contributor

@katejeffreys did you see Polymer/polymer#5023

We're not going to allow arbitrary values for interpolations into templates. Only other templates.

@ghost
Copy link
Author

ghost commented Jan 8, 2018

Thx for heads-up @justinfagnani. Editing now.

@ghost
Copy link
Author

ghost commented Jan 9, 2018

@arthurevans @justinfagnani This is ready for another look.

```

[See a working example in Plunker](http://plnkr.co/edit/AzkMDJ?p=preview).
Polymer 2.4+ provides a helper function (`Polymer.html`) to generate an `HTMLTemplateElement`
instance from a template literal.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do "JavaScript template literal" to be extra clear, since we're also talking about HTML templates too

class MyElement extends Polymer.Element {

static get template() {
return `<style>:host { color: blue; }</style>
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this need to be:

  return html`<style>:host { color: blue; }</style>

Copy link
Author

Choose a reason for hiding this comment

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

@justinfagnani per @arthurevans' feedback, I left that example in for users of pre-2.4 versions.

I may have misunderstood which forms are available in which versions, though. My understanding was that in 2.4+, the string template return value is deprecated but still usable. For that reason the heading indicates that the example applies to "all 2.x versions" - would you mind clarifying on that point?

@arthurevans WDYT?

@ghost
Copy link
Author

ghost commented Jan 11, 2018

This is almost ready to merge, I think. I need a couple points of clarification though:

  • Is a string template still a possible return value of static get template(){...} in 2.4+?
  • Should we keep the string template example in the docs for users of pre-2.4 versions?

@arthurevans @justinfagnani

@kevinpschaaf
Copy link
Member

@katejeffreys

  • Is a string template still a possible return value of static get template(){...} in 2.4+?

It is deprecated (a warning will print), but still allowed. The intention is to actually remove support before the final 3.0 release is tagged.

  • Should we keep the string template example in the docs for users of pre-2.4 versions?

I'd say no, let's switch to using Polymer.html (maybe with a note that this is available in 2.4)?

@arthurevans
Copy link

@kevinpschaaf @katejeffreys For the string template, I think we should avoid giving people the impression that they need to change this now if we're going to provide a we're going to provide an automated transition to 3.0. Plus, some people won't upgrade to 2.4 right away. So we should definitely provide the alternative.

How about we show the Polymer.html version first, then note: Polymer.html was added in release 2.4. For earlier versions, you can return a plain string:

    return `<div>A plain string template</div>`;

Returning a string is deprecated in favor of the html helper in release 2.4. Support for the string template will be removed in 3.0.

... Or words to that effect.

@ghost
Copy link
Author

ghost commented Feb 1, 2018

Copy link

@arthurevans arthurevans left a comment

Choose a reason for hiding this comment

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

LGTM!

called `html`. To enable HTML code highlighting in such text editors, declare a constant called
`html` to hold the `Polymer.html` function. Use the `html` constant in the template getter.

Declare a constant called html to hold the Polymer.html function: { .caption }

Choose a reason for hiding this comment

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

Captions shouldn't end with punctuation (throughout)

@arthurevans arthurevans merged commit 30129ce into master Feb 1, 2018
@ghost ghost deleted the html-helper branch February 20, 2018 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants