Skip to content

feat(style-editor): Improved empty message#35496

Open
KevinDavilaDotCMS wants to merge 2 commits intomainfrom
35466-style-editor-empty-state-redirect-users-to-content-type-schema-definition-when-no-schema-is-configured
Open

feat(style-editor): Improved empty message#35496
KevinDavilaDotCMS wants to merge 2 commits intomainfrom
35466-style-editor-empty-state-redirect-users-to-content-type-schema-definition-when-no-schema-is-configured

Conversation

@KevinDavilaDotCMS
Copy link
Copy Markdown
Member

@KevinDavilaDotCMS KevinDavilaDotCMS commented Apr 28, 2026

  • Added and to manage access to the style editor and permissions tabs based on feature flags and user permissions.
  • Introduced to resolve permissions for displaying the permissions tab.
  • Updated routing to include guards and resolvers for the new tabs.
  • Enhanced component templates to reflect the new tab structure and logic.
  • Added unit tests for guards and resolver to ensure correct functionality.

This implementation improves user experience by conditionally displaying tabs based on user permissions and feature availability.

Proposed Changes

  • change 1
  • change 2

Checklist

  • Tests
  • Translations
  • Security Implications Contemplated (add notes if applicable)

Additional Info

** any additional useful context or info **

Screenshots

Original Updated
** original screenshot ** ** updated screenshot **

This PR fixes: #35466

…d permissions

- Added  and  to manage access to the style editor and permissions tabs based on feature flags and user permissions.
- Introduced  to resolve permissions for displaying the permissions tab.
- Updated routing to include guards and resolvers for the new tabs.
- Enhanced component templates to reflect the new tab structure and logic.
- Added unit tests for guards and resolver to ensure correct functionality.

This implementation improves user experience by conditionally displaying tabs based on user permissions and feature availability.
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 28, 2026

Claude finished @KevinDavilaDotCMS's task in 4m 32s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze and post review

The core logic is sound. A few things worth a second look:

1. No spec for DotUveStyleEditorEmptyStateComponent

The component picked up a new contentTypeVar input, conditional @if rendering, and a RouterLink dependency — none of which are tested. The file doesn't appear in the changed files list and there's no existing spec. The new CTA path (/content-types-angular/edit/{var}/style-editor) and the hide-when-empty behavior are pure render logic but they're also the primary UX addition of this PR.

Suggest adding: input present → link visible with correct routerLink; input absent → link hidden.

2. $styleSchemaContentTypeVar silently returns '' without active contentlet

// edit-ema-editor.component.ts:207-209
readonly $styleSchemaContentTypeVar = computed(
    () => this.uveStore.editorActiveContentlet()?.contentlet?.contentType ?? ''
);

The @if ($contentTypeVar()) guard in the template correctly suppresses the link when there's no active contentlet, so this doesn't produce a broken route. The issue is that the empty state can appear in two distinct states — "no contentlet selected" and "contentlet selected, no schema defined" — and the UX is identical except for the presence of the CTA. If the product intent is to always show the CTA (or always hide it when no contentlet is active), that's fine, but it's worth verifying the intended behavior when the palette is open with nothing selected.

3. Guard makes a fresh HTTP call on every navigation to the guarded route

// dot-content-type-tabs.guard.ts:15-18
return inject(DotPropertiesService)
    .getFeatureFlag(FeaturedFlags.FEATURE_FLAG_UVE_STYLE_EDITOR)
    .pipe(map((enabled) => enabled || redirectToFields(router, state.url)));

The parent route already resolves FeaturedFlags.FEATURE_FLAG_UVE_STYLE_EDITOR via DotFeatureFlagResolver and stores it in route.parent.data['featuredFlags']. Reading from route.parent?.data['featuredFlags']?.[FeaturedFlags.FEATURE_FLAG_UVE_STYLE_EDITOR] would make the guard synchronous (no HTTP, no observable) and consistent with the resolver's result. The author's coupling argument (#1 in their reply) is fair if decoupling is intentional, but it's worth knowing the guard result can silently diverge from the tab visibility if the two sources return different values.

4. Redirect regex is correct

// dot-content-type-tabs.guard.ts:9-10
const redirectToFields = (router: Router, url: string) =>
    router.parseUrl(url.replace(/\/[^/?]+(\?.*)?$/, '/fields$1'));

The test for query-param preservation (?foo=bar) covers the tricky case. ✓

5. contentType field used as route variable

ContentletPayload.contentType is the variable name in DotCMS (confirmed by its use as the withContentTypeCache key and loadContentType(variable) call). The routerLink /content-types-angular/edit/{contentType}/style-editor is correct. ✓

6. Vestigial events: EMPTY in layout spec

// content-types-layout.component.spec.ts:245
provide: ActivatedRoute,
useValue: {
    ...
    events: EMPTY   // Component uses router.events, not route.events
}

The component subscribes to this.#router.events (via Router), not ActivatedRoute.events. The mock is a no-op. Pre-existing issue, not introduced here.


Blocking: None — the guards, resolver, routing, and tab visibility logic are correct. The missing spec (#1) and the HTTP redundancy (#3) are the most actionable items.

@KevinDavilaDotCMS
Copy link
Copy Markdown
Member Author

Response to Claude bot review

Applied #2 (removed dead i18n keys) and #3 (converted to functional ResolveFn).

Skipping the rest:

#1 — Redundant API call: The resolver and guard serve different purposes — the resolver drives tab visibility in the parent template, while the guard protects direct URL access. Collapsing them by reading resolver data from the route snapshot would create tight coupling: if the resolver is ever removed, the guard would silently break. The extra API call is negligible for this path.

#4 — Weak test assertions (not.toBe(false)): The expect(router.parseUrl).toHaveBeenCalledWith(FIELDS_URL) assertion already covers the redirect intent. The not.toBe(false) is harmless redundancy; fixing it is not worth a separate commit.

#5 — Magic string 'permissions': The string appears in exactly two places (guard + resolver, same directory). Extracting it to a constant is not worth the overhead given the scope. If the portlet ID ever changes, both files are trivially discoverable.

#6[innerHTML] with HTML in i18n: Pre-existing pattern, acknowledged as low risk by the bot itself. Not introduced by this PR.

…tional resolver

- Replaced the class-based DotContentTypeTabsResolver with a functional resolver, dotContentTypeTabsResolver, utilizing Angular's ResolveFn.
- Updated unit tests to reflect the new resolver structure and ensure correct functionality.
- Adjusted routing to use the new functional resolver for tab permissions.

This change simplifies the resolver implementation and enhances maintainability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Style Editor empty state: redirect users to content type schema definition when no schema is configured

1 participant