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
perf(ivy): apply static styles/classes directly to an element's style/className properties #33364
Conversation
0daafd4
to
51e498f
Compare
@@ -1853,6 +1854,12 @@ export function textBindingInternal(lView: LView, index: number, value: string): | |||
* style and class entries to the element. | |||
*/ | |||
export function renderInitialStyling(renderer: Renderer3, native: RElement, tNode: TNode) { | |||
renderStylingMap(renderer, native, tNode.classes, true); | |||
renderStylingMap(renderer, native, tNode.styles, false); | |||
if (tNode.classes) { |
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.
Could we add a more strict check here? I presume it would be along the line of tNode.classes !== null
?
const classes = getInitialStylingValue(tNode.classes); | ||
writeStylingValueDirectly(renderer, native, classes, true, null); | ||
} | ||
if (tNode.styles) { |
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.
Could we add a more strict check here? I presume it would be along the line of tNode. styles !== null
?
|
||
function objectToClassName(obj: {[key: string]: any} | null): string { | ||
let str = ''; | ||
if (obj) { |
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.
Could we move this check to the caller (preferred) or, as the very minimum, make it more explicit (test for !== null
)
@@ -888,6 +921,26 @@ export const setClass: ApplyStylingFn = | |||
} | |||
}; | |||
|
|||
export const setClassName = (renderer: Renderer3 | null, native: RElement, className: string) => { | |||
if (renderer !== null) { |
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'm surprised that we are testing for the renderer's presence here. Under what condition it can get null
?
51e498f
to
3cc0059
Compare
[MASTER]
[THIS PR]
|
…/className properties
3cc0059
to
730ee6f
Compare
…s style/className properties
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
…s style/className properties
7527437
to
c018b5d
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
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. |
No description provided.