[Angular CSDK] Basic rendering finalize#461
[Angular CSDK] Basic rendering finalize#461art-alexeyenko wants to merge 3 commits intofeature/angular-csdkfrom
Conversation
art-alexeyenko
commented
May 3, 2026
art-alexeyenko
commented
- Adjust Tailwind integration a little
- Fix styles and style application via component props
- Add dedicated router link directive
- Miscellaneous fixes for Angular template
- Most of template logic will be moved out repo and transformed into separate sample app closer to 1.0
…onents - part 2 (with routerLink)
illiakovalenko
left a comment
illiakovalenko
left a comment
There was a problem hiding this comment.
Looks good overall thanks for the work here 👍
I’ve left a few suggestions below, and also have a couple of follow-ups:
- Could you enable a code coverage report for the Angular SDK?
- Once coverage is enabled, please review the results and add unit tests where needed. I’ve already highlighted some missing scenarios in the comments.
- Could you also address the lint errors and warnings? There are quite a few at the Angular SDK level.
- Regarding the components implemented at the template level: should we treat this as a draft for the SCB team? In particular, there are some utilities included that likely shouldn’t remain there long term.
Let me know your thoughts.
|
|
||
| readonly styles = () => { | ||
| const s = this.params()?.['styles']; | ||
| const p = { ...this.rendering()?.params, ...this.params() }; |
There was a problem hiding this comment.
Looks like this path is only partially covered by tests.
Current specs validate rendering.params usage, but I don’t see a case where params input is provided directly and merged with rendering.params.
Could we add a unit test for direct params input (and ideally precedence when both are set)?
| export class ScFormComponent { | ||
| readonly rendering = input<ComponentRendering>(); | ||
| readonly params = input<{ [key: string]: string }>({}); | ||
| readonly fields = input<{ [key: string]: unknown }>({}); |
There was a problem hiding this comment.
fields input is defined but not used
| if (link.target) { | ||
| renderer.setAttribute(element, 'target', link.target); | ||
| if (link.target === '_blank' && !element.getAttribute('rel')) { | ||
| renderer.setAttribute(element, 'rel', 'noopener noreferrer'); |
There was a problem hiding this comment.
I think rel="noopener noreferrer" can become stale after target changes away from _blank.
There is no symmetric removal when target is later cleared or changed.
| if (!hasChildren) { | ||
| const text = link.text || link.href || ''; | ||
| renderer.setProperty(element, 'textContent', text); | ||
| } else if (options.preferTextFromField && link.text) { |
There was a problem hiding this comment.
preferTextFromField is not covered by unit tests
There was a problem hiding this comment.
Some functions are not covered by unit tests:
- applyLinkFieldToAnchor
- clearLinkHrefOnAnchor
| * Clears link-driven attributes when the field is empty (matches ScLink behavior: drop `href` only). | ||
| */ | ||
| export function clearLinkHrefOnAnchor(renderer: Renderer2, element: HTMLAnchorElement): void { | ||
| renderer.removeAttribute(element, 'href'); |
There was a problem hiding this comment.
Do we need to cleanup field-empty transitions of other link-driven attrs (target, title, classes, maybe rel)?
|
|
||
| /** | ||
| * Renders a Sitecore link field onto a host `<a>` and calls `Router.navigateByUrl` on click, | ||
| * following the Sitecore JSS Angular `RouterLinkDirective` pattern (sitecore-jss-angular). |
There was a problem hiding this comment.
Let’s avoid mentioning JSS, as it may not provide meaningful context for new consumers. I think the name of this component is self explaining
| // return; | ||
| // } | ||
|
|
||
| void this.router.navigateByUrl(hrefAttr); |
There was a problem hiding this comment.
Should we think about guarding router link directive in case content authors pass external url? Most likely 'onClick' behavior will not be functional
| spy.mockRestore(); | ||
| }); | ||
|
|
||
| it('calls navigateByUrl for href with hash and does not preventDefault (JSS parity)', async () => { |
There was a problem hiding this comment.
In general all the functionality should fully match JSS functional requirements, so no need to mention it
| it('calls navigateByUrl for href with hash and does not preventDefault (JSS parity)', async () => { | |
| it('calls navigateByUrl for href with hash and does not preventDefault', async () => { |
There was a problem hiding this comment.
We are missing some 'navigation' scenarios:
- target="_blank" behavior
- missing/empty href path (no navigation)
- external/protocol URLs (should not router-navigate, if you adopt guardrails)
2 participants
