feat(projection): Host Projection service #1756
Conversation
googlebot
added
the
cla: yes
hansl
force-pushed
the
projection
branch
4 times, most recently
from
07f8973 to
75c2693
Compare
hansl
added
pr: needs review
and removed
in progress
| </div> | ||
| `, | ||
| styles: [` | ||
| .outer { |
There was a problem hiding this comment.
Prefix class name with demo-
(same for other classes in the demo)
|
|
||
|
|
||
| @Injectable() | ||
| export class MdProjectionService { |
There was a problem hiding this comment.
How about DomProjector?
I'm generally opposed to the Service suffix since it's somewhat meaningless; it's talking about how the class is meant to be consumed rather than what it does. Also, the service is specifically dealing with projection via DOM, hence DomProjector.
Going along this vein, how about calling the directive DomProjectionHost with selector dom-projection-host? I think this makes the API a bit more explicit than <md-host>, which doesn't give a string cue as to what it's for.
(also omitting the md prefix since this would be something not strictly related to material design, similar to the portal and overlay directives).
| let child = childNodes[0]; | ||
|
|
||
| // Replace ComponentEl by all its children, in place. | ||
| this._replaceWith(componentEl, child); |
There was a problem hiding this comment.
Make this just an non-exported module function?
| * Project an element into a host element. | ||
| */ | ||
| project(ref: ElementRef, host: MdProjectionHostDirective): void { | ||
| const componentEl = ref.nativeElement; |
|
|
||
| @Injectable() | ||
| export class MdProjectionService { | ||
| private _replaceWith(toReplaceEl: HTMLElement, otherEl: HTMLElement) { |
There was a problem hiding this comment.
Could be Node instead of HTMLElement (less specific)
There was a problem hiding this comment.
We cannot replace all nodes by all nodes in the tree. Element is semantically accurate.
|
|
||
| // Replace ComponentEl by all its children, in place. | ||
| this._replaceWith(componentEl, child); | ||
| while (childNodes.length) { |
There was a problem hiding this comment.
Reading length on a NodeList is notoriously slow; it's better to cache the length and for loop.
(here and below)
| } | ||
|
|
||
| /** | ||
| * Project an element into a host element. |
There was a problem hiding this comment.
This comment needs some expansion; the key thing to communicate is that it's only the given element that is projected and not its content (children). Also worth mention when you might want to use this.
| * Project an element into a host element. | ||
| */ | ||
| project(ref: ElementRef, host: MdProjectionHostDirective): void { | ||
| const componentEl = ref.nativeElement; |
There was a problem hiding this comment.
componentEl -> projectedEl? (since it's not necessarily a component)
| const childNodes = componentEl.childNodes; | ||
| let child = childNodes[0]; | ||
|
|
||
| // Replace ComponentEl by all its children, in place. |
There was a problem hiding this comment.
Expand comment to something like
// We hoist all of the projected element's children out into the projected elements position
// because we *only* want to move the projected element and not its children.| `, | ||
| styles: [` | ||
| .inner { | ||
| background-color: blue; |
There was a problem hiding this comment.
nit: these colors are totally unreadable to me (when I run the demo)
|
@jelbourn please re-review |
jelbourn
added
pr: lgtm
action: merge
|
@hansl I think you need to add |
|
@kara could you rerun the presubmits? It should pass now. |
|
Absolutely! Momentito. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Uh oh!
There was an error while loading. Please reload this page.