Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: RendererV2 #14469

Merged
merged 1 commit into from Feb 15, 2017
Merged

feat: RendererV2 #14469

merged 1 commit into from Feb 15, 2017

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Feb 14, 2017

No description provided.

@@ -85,16 +85,26 @@ export class DebugElement extends DebugNode {
insertChildrenAfter(child: DebugNode, newChildren: DebugNode[]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use insertBefore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also add the breaking change in the commit msg

Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

@@ -7,4 +7,4 @@
*/

// Public API for render
export {RenderComponentType, Renderer, RootRenderer} from './render/api';
export {RENDERER_V2_TOKEN, RenderComponentType, Renderer, RendererV2, RootRenderer} from './render/api';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

RENDERER_V2_IMPL

@@ -137,7 +137,12 @@ export function createElement(view: ViewData, renderHost: any, def: NodeDef): El
if (view.parent || !rootSelectorOrNode) {
const parentNode =
def.parent != null ? asElementData(view, def.parent).renderElement : renderHost;
el = elDef.name ? renderer.createElement(elDef.name) : renderer.createComment('');
if (elDef.name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add todo: move in the def

Copy link
Contributor

Choose a reason for hiding this comment

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

I can fix it later on.

@@ -146,7 +151,8 @@ export function createElement(view: ViewData, renderHost: any, def: NodeDef): El
}
if (elDef.attrs) {
for (let attrName in elDef.attrs) {
renderer.setAttribute(el, attrName, elDef.attrs[attrName]);
const nsAndName = splitNamespace(attrName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add todo everywhere

@@ -180,6 +180,12 @@ export enum RenderNodeAction {

export function visitRootRenderNodes(
view: ViewData, action: RenderNodeAction, parentNode: any, nextSibling: any, target: any[]) {
// TODO: this should be done inside of the check for nodeDef.type === Nodetype.Element, ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove the todo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

explain why, ie used when manually moving view nodes around

@vicb vicb changed the title feat: RendererV2 [WIP] feat: RendererV2 Feb 14, 2017

removeClass(el: any, name: string): void { el.classList.remove(name); }

setStyle(el: any, style: string, value: any): void { el.style[style] = value; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

and support for vendor prefixed/css vars #12847

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up, I'll add the params

@vicb vicb changed the title [WIP] feat: RendererV2 feat: RendererV2 Feb 14, 2017
*
* @experimental
*/
export const RENDERER_V2_IMPL = new InjectionToken<RendererV2>('Renderer V2');
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to RENDERER_V2_DIRECT

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 14, 2017
@vicb vicb force-pushed the 0208-renderv2 branch 3 times, most recently from 0e532ff to 1106d20 Compare February 14, 2017 21:31
@vicb vicb added area: core Issues related to the framework runtime and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 14, 2017

removeClass(el: any, name: string): void { el.classList.remove(name); }

setStyle(el: any, style: string, value: any, hasVendorPrefix: boolean, hasImportant: boolean):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think signature should be
setStyle(el: any, style: string, value: any, priority?: string)
and impl

setStyle(el: any, style: string, value: any, priority?: string) {
   if (style[0] === '-' || priority) {
     el.style.setProperty(style, value, priority);
   } else {
    el.style[style] = value;
   }
}

priority?: string isntead of boolean because native setProperty wants a string.
hasVendorPrefix I think is not necessary because it's just a simple check style[0] === '-'

@vicb vicb added the action: merge The PR is ready for merge by the caretaker label Feb 14, 2017
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGMT - but I see that the api docs are missing. we should add these before the final release.

@IgorMinar IgorMinar merged commit bb9c7ae into angular:master Feb 15, 2017
@vicb vicb deleted the 0208-renderv2 branch February 16, 2017 18:56
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants