-
Notifications
You must be signed in to change notification settings - Fork 2.4k
3.0-ify shadow-dom, style-shadow-dom, and custom-css-properties #2476
Conversation
ghost
commented
Feb 20, 2018
•
edited by ghost
Loading
edited by ghost
- shadow dom
- style shadow dom
- custom css props
app/3.0/docs/devguide/shadow-dom.md
Outdated
```html | ||
<link rel="import" href="/bower_components/polymer/lib/utils/flattened-nodes-observer.html"> | ||
```js | ||
import * from '@polymer/polymer/utils/flattened-nodes-observer.js'; |
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.
check 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.
I don't think this is a valid format.
If the module has a default export (most of ours don't), you can do:
import defaultExport from "module specifier"
To create a new object that contains references to all of the exports from a module, you can do:
import * as myNameforFNO from '@Polymer.../flattened-nodes-observer.js';
Then use myNameforFNO.FlattenedNodesObserver;
Or you could just import individual symbols exported from the module:
import {FlattenedNodesObserver} from 'module specifier';
probably needs some iteration but ready for a look @arthurevans https://p3-shadow-dom-concepts-dot-polymer-project.appspot.com/ |
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.
Left some notes. Sorry these are so late--forgot to submit earlier.
<!-- import the custom-style polyfill to prevent "leaks" in some browsers --> | ||
<script type="module" src="./node_modules/@polymer/polymer/lib/elements/custom-style.js"></script> | ||
|
||
<!-- wrap document-level styles to avoid "leaks" in some browsers --> |
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 think this comment is misleading. Here you're using the <custom-style>
to ensure that the custom properties are shimmed (polyfilled) on browsers that don't support them natively. I don't think leaking is an issue here (maybe @azakus knows better)...
``` | ||
|
||
## Create custom properties | ||
If you provide documentation for the custom properties your element provides, users won't need to know any implementation details. See [Documenting your elements](/{{{polymer_version_dir}}}/docs/tools/documentation) for more information, or take a look at the [documentation for the Polymer `paper-ui-elements`](https://www.webcomponents.org/collection/PolymerElements/paper-ui-elements) for examples. |
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.
"won't" => "don't" real tiny nit here, but should probably be present tense, for simplicity.
<!-- import the custom-style polyfill --> | ||
<script type="module" src="node_modules/@polymer/polymer/lib/elements/custom-style.js"></script> | ||
|
||
<!-- wrap document-level styles with the custom-style polyfill to prevent style "leak" in some browsers --> |
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.
nit: custom-style isn't a polyfill, it's an element that makes sure the contained styles get polyfilled.
As above, it may be more relevant that the custom-style element invokes the custom properties polyfill, which polyfills custom property support on browsers that don't have it (as well as the shady scoping polyfill, which prevents style leaks, as described).
Don't need the is="custom-style" anymore--3.x code can't run in 1.x, so we don't need hybrid syntax.
|
||
<!-- wrap document-level styles to avoid "leaks" in some browsers --> | ||
<custom-style> | ||
<style is="custom-style"> |
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.
Shouldn't need the 'is="custom-style" anymore.
```html | ||
... | ||
<custom-style> | ||
<style is="custom-style"> |
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.
No is="custom-style"
``` | ||
|
||
[See it on Plunker](http://plnkr.co/edit/hR3I4w?p=preview) | ||
|
||
## Share styles between elements | ||
|
||
### Use style modules {#style-modules} | ||
|
||
The preferred way to share styles is with *style modules*. You can package up styles in a style module, and share them between elements. | ||
|
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.
Going forward, we're probably going to recommend using template mechanism rather than style modules. We should add that as a separate update after this one, though, I think.
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.
ok, created #2513 to track
</template> | ||
</dom-module> | ||
```js | ||
static template get() { |
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.
static get template()
``` | ||
|
||
## Use `custom-style` in document-level styles {#custom-style} | ||
`custom-style` is not included in the `@polymer/polymer/polymer-element.js` package. Import `custom-style` from `@polymer/polymer/lib/elements/custom-style.js`: |
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.
package => module
<body> | ||
<custom-element></custom-element> | ||
</body> | ||
``` | ||
|
||
*Note: You should only use `custom-style` to define styles for the main document. To define styles for an element's local DOM, just use a `<style>` block.* |
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.
Fix note formatting: Only bold run-in head, add {.alert .alert-info}
... Extra credit, replace Note with a more informative run-in so people know whether they should read it. Like:
Don't use custom-style inside an element's template.
also: local DOM => shadow DOM
|
||
*Note: You should only use `custom-style` to define styles for the main document. To define styles for an element's local DOM, just use a `<style>` block.* | ||
|
||
### Examples | ||
|
||
In the first code sample, the style for the `p` element “leaks” into Paragraph B in browsers that haven’t implemented the Shadow DOM v1 specs. In the second code sample, the developer has used `custom-style` to wrap the style block, preventing this leak. | ||
In the following code sample, the document-level style in index.html "leaks" into the shadow DOM of `<custom-element>` in browsers that haven’t implemented the Shadow DOM v1 specs. |
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.
Note for the future: at some point (not right now) we should clarify what we mean by "leaking." It could be confusing, because inheritance is also a form of "leaking," right?
Specifically, what we mean when we say styles can't leak is that _a selector in one scope (i.e., in the main document) can't match an element in a different scope (like in a shadow root)... Except in a couple of very specific instances, namely :host and ::slotted() ...
Anyway, think on it... We should convey that at some point.
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.
agree, opened #2514
@arthurevans Thanks! Incorporated feedback |
@arthurevans would you mind taking another look - there were a couple of missing parts d9d4ff6 |
Reviewed added parts, LGTM. Merging. |