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 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/odd-ducks-joke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@builder.io/sdk-angular": patch
---

Fix: handle the rendering of unknown HTML elements.
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import { expect } from '@playwright/test';
import { excludeTestFor, test } from '../helpers/index.js';
import { test } from '../helpers/index.js';

test.describe('Duplicate Attributes', () => {
test('wrapped block has no duplicate attributes', async ({ page, packageName, sdk }) => {
test.fail(
excludeTestFor({ angular: true }, sdk),
'attributes not spread out so cant set footer'
);
test('wrapped block has no duplicate attributes', async ({ page, packageName }) => {
test.skip(packageName === 'react-native-74' || packageName === 'react-native-76-fabric');

await page.goto('/duplicate-attributes');

const footer = await page.locator('footer');
Expand Down
16 changes: 16 additions & 0 deletions packages/sdks-tests/src/e2e-tests/dynamic-element.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { expect } from '@playwright/test';
import { test } from '../helpers/index.js';

test.describe('Unknown Element', () => {
test('unknown element is rendered', async ({ page, sdk }) => {
test.skip(sdk !== 'angular');
await page.goto('/dynamic-element');

const tagNameLocator = await page.locator('test');
const tagNameId = await tagNameLocator.getAttribute('builder-id');

expect(tagNameId).toBe('builder-493100cea9504d56886045462b65b481');
await expect(tagNameLocator).toHaveAttribute('class','builder-493100cea9504d56886045462b65b481 builder-block');
await expect(tagNameLocator).toHaveText('testing text');
});
});
47 changes: 47 additions & 0 deletions packages/sdks-tests/src/specs/dynamic-element.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
export const DYNAMIC_ELEMENT ={
"data": {
"title": "angular-test-1",
"themeId": false,
"blocks": [
{
"@type": "@builder.io/sdk:Element",
"@version": 2,
"tagName": "test",
"id": "builder-493100cea9504d56886045462b65b481",
"component": { "name": "Text", "options": { "text": "testing text" } },
"responsiveStyles": {
"large": {
"display": "flex",
"flexDirection": "column",
"position": "relative",
"flexShrink": "0",
"boxSizing": "border-box",
"marginTop": "20px",
"lineHeight": "normal",
"height": "auto"
}
}
},
{
"@type": "@builder.io/sdk:Element",
"@version": 2,
"tagName": "p",
"id": "builder-f6bddd39177d490fb278ad6516fa6a75",
"component": { "name": "Text", "options": { "text": "2222" } },
"responsiveStyles": {
"large": {
"display": "flex",
"flexDirection": "column",
"position": "relative",
"flexShrink": "0",
"boxSizing": "border-box",
"marginTop": "20px",
"lineHeight": "normal",
"height": "auto"
}
}
}
]
}
}

2 changes: 2 additions & 0 deletions packages/sdks-tests/src/specs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import {
} from './variant-containers.js';
import { EMBED_AND_CUSTOM_CODE } from './embed-and-custom-code.js';
import { COLUMNS_VERTICAL_CENTER_FLEX } from './columns-vertical-center-flex.js';
import { DYNAMIC_ELEMENT } from './dynamic-element.js';

function isBrowser(): boolean {
return typeof window !== 'undefined' && typeof document !== 'undefined';
Expand Down Expand Up @@ -271,6 +272,7 @@ export const PAGES: Record<string, Page> = {
},
'/columns-vertical-center-flex': { content: COLUMNS_VERTICAL_CENTER_FLEX },
'/can-track-false-pre-init': { content: HOMEPAGE, target: 'gen1' },
'/dynamic-element': { content: DYNAMIC_ELEMENT },
} as const;

export type Path = keyof typeof PAGES;
Expand Down
96 changes: 81 additions & 15 deletions packages/sdks/output/angular/scripts/generate-dynamic-renderer.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -76,16 +76,11 @@ const dynamicComponentTemplate = (tagName) => {
export class Dynamic${capitalize(tagName)} {
@Input() attributes!: any;
@Input() actionAttributes?: any;

@ViewChild('v', { read: ElementRef }) v!: ElementRef;

_listenerFns = new Map<string, () => void>();

constructor(private renderer: Renderer2) {}

setAttributes(el: any, value: any, changes?: any) {
if (!el) return;

const target = changes ? changes : value;
Object.keys(target).forEach((key) => {
if (key.startsWith('on')) {
Expand All @@ -98,17 +93,14 @@ export class Dynamic${capitalize(tagName)} {
}
});
}

ngAfterViewInit() {
this.setAttributes(this.v?.nativeElement, this.attributes);
}

ngOnChanges(changes: SimpleChanges) {
if (changes.attributes) {
this.setAttributes(this.v?.nativeElement, this.attributes, changes.attributes.currentValue);
}
}

ngOnDestroy() {
this._listenerFns.forEach(fn => fn());
}
Expand All @@ -132,19 +124,33 @@ const generateComponents = () => {
TagName;
inputs: {
attributes: attributes,
actionAttributes: actionAttributes
actionAttributes: actionAttributes,
tagName: tagName
};
content: myContent
"
></ng-container>
</ng-container>
<ng-container *ngIf="!(useTypeOf(TagName) === 'string') && getClassName() === 'DynamicElement'">
<ng-container
*ngComponentOutlet="
TagName;
inputs: {
attributes: attributes,
actionAttributes: actionAttributes,
tagName: tagName
};
content: myContent
"
></ng-container>
</ng-container>
<ng-container *ngIf="!(useTypeOf(TagName) === 'string')">
<ng-container *ngIf="!(useTypeOf(TagName) === 'string') && getClassName() === ''">
<ng-container
*ngComponentOutlet="
TagName;
inputs: {
attributes: attributes,
actionAttributes: actionAttributes
actionAttributes: actionAttributes,
};
content: myContent
"
Expand All @@ -157,7 +163,7 @@ const generateComponents = () => {
TagName;
inputs: {
attributes: attributes,
actionAttributes: actionAttributes
actionAttributes: actionAttributes,
};
content: myContent
"
Expand All @@ -174,22 +180,31 @@ export default class DynamicRenderer {
@Input() TagName!: any;
@Input() attributes!: any;
@Input() actionAttributes!: any;
@Input() tagName : string;

@ViewChild('tagnameTemplate', { static: true }) tagnameTemplateRef!: TemplateRef<any>;

className: string = '';
myContent?: any[][];

useTypeOf(obj: any): string {
return typeof obj;
}

getClassName(): string {
return this.className;
}

constructor(private vcRef: ViewContainerRef) {}

ngOnInit() {
if (typeof this.TagName === 'string') {
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should remove these hard-coded dynamic elements from dynamic renderer and use this dynamic-unknown-element to render dynamic elements. We could rename this to dynamic-element also if it makes more sense or even better we could refactor it so that we handle this inside the dynamic renderer itself (because that's the role of dynamic-renderer) but I think for now removing the switch and handling all the elements inside dynamic-element will be nice. What do you think?

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 totally makes sense

default:
this.tagName = this.TagName;
this.TagName = DynamicElement;
this.className = 'DynamicElement';
break;
}
}
Expand All @@ -198,9 +213,60 @@ export default class DynamicRenderer {
}
`;

htmlElements.forEach((tagName) => {
dynamicRendererCode += dynamicComponentTemplate(tagName) + '\n';
});
htmlElements.forEach((tagName) => {
dynamicRendererCode += dynamicComponentTemplate(tagName) + '\n';
});

dynamicRendererCode += `
@Component({
selector: 'dynamic-element',
template: '<ng-content></ng-content>',
standalone: true
})

export class DynamicElement {
@Input() tagName: string;
@Input() attributes: any;
@Input() actionAttributes: any;

private _element!: HTMLElement;
private _listenerFns = new Map<string, () => void>();

constructor(private hostRef: ElementRef, private renderer: Renderer2) {}

ngOnInit() {
if(this.tagName){
this._element = this.renderer.createElement(this.tagName);
this.renderer.appendChild(this.hostRef.nativeElement, this._element);
this.setAttributes(this._element, this.attributes);
}
}

ngAfterViewInit(){
if(this.hostRef.nativeElement.children.length > 1){
this.renderer.appendChild(this.hostRef.nativeElement.children[1], this.hostRef.nativeElement.children[0]);
}
}

ngOnDestroy() {
this._listenerFns.forEach((fn) => fn());
}

setAttributes(el: any, attributes: any) {
if (!attributes) return;
Object.keys(attributes).forEach((key) => {
if (key.startsWith('on')) {
if (this._listenerFns.has(key)) {
this._listenerFns.get(key)!();
}
const eventType = key.replace('on', '').toLowerCase();
this._listenerFns.set(key, this.renderer.listen(el, eventType, attributes[key]));
} else {
this.renderer.setAttribute(el, key, attributes[key] ?? '');
}
});
}
}`

fs.writeFileSync(PATH_TO_DYNAMIC_RENDERER, dynamicRendererCode);

Expand Down
2 changes: 1 addition & 1 deletion packages/sdks/src/components/block/block.lite.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export default function Block(props: BlockProps) {
* eslint-disable-next-line @typescript-eslint/ban-ts-comment
* @ts-ignore */
reactNative: View,
angular: DynamicDiv,
angular: props.block.tagName || DynamicDiv,
default: props.block.tagName || 'div',
});
},
Expand Down
Loading