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: ENG-8176 fallback UI for unknown elements #3892

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

shubham-builder
Copy link
Contributor

Description

Added fallback UI for unknown element in angular dynamic renderer

Screenshot
If relevant, add a screenshot or two of the changes you made.

Copy link

changeset-bot bot commented Feb 4, 2025

🦋 Changeset detected

Latest commit: 8d87cce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@builder.io/sdk-angular Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

nx-cloud bot commented Feb 4, 2025

View your CI Pipeline Execution ↗ for commit 8d87cce.

Command Status Duration Result
nx test @e2e/react-sdk-next-14-app ❌ Failed 4m 27s View ↗
nx test @e2e/solid-start ❌ Failed 3m 54s View ↗
nx test @e2e/angular-16 ❌ Failed 3m 54s View ↗
nx test @snippet/angular-16 ❌ Failed 3m 15s View ↗
nx test @snippet/angular-16-ssr ❌ Failed 2m 40s View ↗
nx test @e2e/nextjs-sdk-next-app ✅ Succeeded 7m 54s View ↗
nx test @e2e/qwik-city ✅ Succeeded 7m 55s View ↗
nx test @e2e/nuxt ✅ Succeeded 7m 22s View ↗
Additional runs (36) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-02-14 16:28:25 UTC

@yash-builder
Copy link
Contributor

yash-builder commented Feb 4, 2025

Hey, @shubham-builder. you can use this command to run @snippet locally. yarn g:nx test @snippet/<name-of-the-package>

Copy link
Contributor

@samijaber samijaber left a comment

Choose a reason for hiding this comment

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

@shubham-builder can you add a test case with an example of such an "unknown element" so we can confirm that this PR fixes it?

@shubham-builder
Copy link
Contributor Author

@sidmohanty11 @samijaber Updated the code so the unknown-element is placed correctly within the dynamic-renderer. Also added content assertion check in the test.

@@ -190,6 +195,8 @@ export default class DynamicRenderer {
switch (this.TagName) {
${htmlElements.map((el) => `case '${el}': this.TagName = Dynamic${capitalize(el)}; break;`).join('\n ')}
Copy link
Contributor

Choose a reason for hiding this comment

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

with this feature now, we should be able to get rid of manual creation of components and this line here (entire switch case)

Copy link
Contributor

Choose a reason for hiding this comment

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

This should also fix Angular 16 tests throwing Input annotation errors saying TagName is not defined in other dynamic components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would mean using the dynamic-unknown-element for all the known elements as well
So in this case even dynamic-p will render as dynamic-unkonwn-element unless it's code is updated accordingly with support of void elements.
Do we want that in same pr or I think we can take it as separate task to eliminate this switch case and use single function to handle known and unknown elements.

}

ngAfterViewInit(){
this.renderer.appendChild(this.hostRef.nativeElement.children[1], this.hostRef.nativeElement.children[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this work? Could you please explain? Can we somehow do this inside ngOnInit? I'm afraid if we'll lose SSRing children with this

Copy link
Contributor Author

@shubham-builder shubham-builder Feb 14, 2025

Choose a reason for hiding this comment

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

Parent component populates the and the is created inside the ngOnInit (which is necessary as input are initialised there). This changes the order in which the elements should be inserted.
So once the view is created this fixes the order of elements( moving the element to be child of unknown element)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants