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

fix(ivy): set namespace for host elements of dynamically created components #35136

Conversation

AndrewKushnir
Copy link
Contributor

Prior to this change, element namespace was not set for host elements of dynamically created components that resulted in incorrect rendering in a browser. This commit adds the logic to pick and set correct namespace for host element when component is created dynamically.

Fixes #34256, #34866.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! A few notes below

@AndrewKushnir AndrewKushnir marked this pull request as ready for review February 4, 2020 18:48
@AndrewKushnir
Copy link
Contributor Author

Hi @kara, thanks for the review! I performed additional changes based on your feedback. Could you pleaser have another look when you get a chance? Thank you.

…onents

Prior to this change, element namespace was not set for host elements of dynamically created components that resulted in incorrect rendering in a browser. This commit adds the logic to pick and set correct namespace for host element when component is created dynamically.
@AndrewKushnir AndrewKushnir force-pushed the namespace-for-dynamic-component-host branch from 12b83e1 to b132e5d Compare February 4, 2020 22:22
@AndrewKushnir
Copy link
Contributor Author

Presubmit

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Feb 4, 2020
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -174,7 +174,7 @@ class DefaultDomRenderer2 implements Renderer2 {
setAttribute(el: any, name: string, value: string, namespace?: string): void {
if (namespace) {
name = namespace + ':' + name;
// TODO(benlesh): Ivy may cause issues here because it's passing around
// TODO(FW-811): Ivy may cause issues here because it's passing around
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating these 👍

@kara kara removed the action: presubmit The PR is in need of a google3 presubmit label Feb 8, 2020
@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Feb 8, 2020
@kara kara closed this in ae0253f Feb 8, 2020
kara pushed a commit that referenced this pull request Feb 8, 2020
…onents (#35136)

Prior to this change, element namespace was not set for host elements of dynamically created components that resulted in incorrect rendering in a browser. This commit adds the logic to pick and set correct namespace for host element when component is created dynamically.

PR Close #35136
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
…onents (angular#35136)

Prior to this change, element namespace was not set for host elements of dynamically created components that resulted in incorrect rendering in a browser. This commit adds the logic to pick and set correct namespace for host element when component is created dynamically.

PR Close angular#35136
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 17, 2020
…onents (angular#35136)

Prior to this change, element namespace was not set for host elements of dynamically created components that resulted in incorrect rendering in a browser. This commit adds the logic to pick and set correct namespace for host element when component is created dynamically.

PR Close angular#35136
@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 Mar 10, 2020
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 cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RC5 IVY] Nested SVG doesn't work anymore
3 participants