Skip to content

fix(edit-content): style comment Send/Clear buttons and add empty-message validation - #36464

Merged
jcastro-dotcms merged 5 commits into
mainfrom
issue-36372-comment-section-button-styles-validation
Jul 9, 2026
Merged

fix(edit-content): style comment Send/Clear buttons and add empty-message validation#36464
jcastro-dotcms merged 5 commits into
mainfrom
issue-36372-comment-section-button-styles-validation

Conversation

@jcastro-dotcms

@jcastro-dotcms jcastro-dotcms commented Jul 7, 2026

Copy link
Copy Markdown
Member

Summary

Closes #36372.

Updates the Comments section of the new Edit Contentlet sidebar to match the requested button styling and adds client-side validation for empty comments.

  • Send is now a primary button; Clear is a tertiary (text/ghost) button — matching the pattern already used for de-emphasized secondary actions elsewhere in the app.
  • Both buttons stay enabled at all times regardless of textarea content. They're still disabled while a save request is in flight, to prevent double-submits — that's request state, not input state, so it isn't affected by the "enabled at all times" requirement.
  • Clicking Send with an empty or whitespace-only comment no longer submits: the textarea gets a red border and "A message is required." renders below it. This clears as soon as the user types a non-empty value or clicks Clear.
  • Added a new commentRequired validation error key (instead of reusing Angular's built-in required) so this field shows our specific copy instead of the shared, generic "This field is required" string used elsewhere in the app.
  • Fixed a latent ordering bug in onSubmit(): markAsDirty()/markAsTouched() must run before setErrors(), since the shared dot-field-validation-message component recomputes its visibility synchronously off the setErrors() emission. With the old order, the message could silently never render even though the field's error state was otherwise correct — this applied to the original required error too, it just wasn't previously covered by any test that checked the rendered text.
  • Moved dot-field-validation-message out of absolute positioning into normal document flow so it no longer visually overlaps the character counter above it.

Test plan

  • Updated/added unit tests in dot-edit-content-sidebar-activities.component.spec.ts (38/38 passing): button enabled/disabled states, required-error message content, error clearing on input/Clear.
  • Verified manually end-to-end in the running app (local Docker backend + nx serve dotcms-ui): button styling, empty-submit validation, error-clears-on-type, Clear dismisses the error, valid comment submits successfully.
  • eslint clean on all touched files.

🤖 Generated with Claude Code

This PR fixes: #36372

…sage validation

Send is now a primary button and Clear a tertiary (text) button, both always
enabled. Submitting an empty or whitespace-only comment now blocks the
request and shows a red border plus "A message is required." message below
the textarea, which clears once the user types or clicks Clear.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jcastro-dotcms's task in 3m 34s —— View job


Code Review

Reviewed the full diff (source + tests + language props) against origin/main.

  • Read changed component HTML/TS and shared dot-field-validation-message changes
  • Verified test coverage for new behavior
  • Checked PrimeNG conventions and logic/edge cases

New Issues

No issues found.

The changes are well-scoped and the logic holds up:

  • onSubmit() ordering fix (dot-edit-content-sidebar-activities.component.ts:233-235) is correct. markAsDirty()/markAsTouched() don't emit statusChanges; only setErrors() does. Since dot-field-validation-message recomputes visibility synchronously off that emission and its template gates on _field.dirty, the dirty/touched flags must be set first. Reasoning checks out.
  • requiredErrorMessage input (dot-field-validation-message.component.ts:49-50, 131) mirrors the existing patternErrorMessage pattern, is optional (falls back to the default key via ||), and is scoped to the required case only — no behavior change for the ~25 other consumers. The regression test confirming it doesn't leak into a pattern error is a good addition.
  • severity="primary" / [text]="true" are valid PrimeNG values already used elsewhere in the codebase.
  • Error-clears-on-type works because the field's custom validator returns null for empty input, so valueChanges → revalidation clears the manually-set required error. Covered by tests.
  • Language key edit.content.sidebar.activities.required is present and matches the binding.

Minor notes (non-blocking)

  • 🟡 dot-edit-content-sidebar-activities.component.html:118 — the fixed-height slot (h-3.5 ≈ 14px, no overflow-hidden) assumes the message stays single-line. "A message is required." is short so this is fine today, but a longer future message (or narrow viewport) could wrap to two lines and overflow the reserved height, re-introducing the footer shift this fix targets. Not a bug now — just a latent assumption worth a comment or overflow-hidden if the copy ever changes.
  • The over-max-length path ({ invalid: true }, value not a string → no message key) renders an empty <small> with no copy. This is pre-existing behavior (the validator and the missing message key predate this PR, and the submit button was already reachable for >500-char input), so it's out of scope here — flagging only for awareness since the always-enabled button makes empty-submit more prominent.

The concern @nicobytes raised about the commentRequired custom key was resolved correctly in dd0f0fa by reverting to the built-in required key plus the scoped requiredErrorMessage override — that's the right call and avoids the timing issue with a conditional message binding.

Approving in spirit — nothing blocking merge.
· issue-36372-comment-section-button-styles-validation

@nicobytes

Copy link
Copy Markdown
Member

The commentRequired custom error key works, but semantically it duplicates Angular's built-in required — both mean "this field is required," the only difference is the copy. We're using the custom validator path just to change the message for what is really a standard required error. A couple of alternatives that avoid introducing a competing error key:

Option A — use the message input that dot-field-validation-message already exposes (smallest change, nothing shared touched):

this.commentControl.setErrors({ required: true });
<dot-field-validation-message
    [field]="form.get('comment')"
    [message]="'edit.content.sidebar.activities.required' | dm" />

In getErrors() the component returns this.defaultMessage ? this.defaultMessage : ..., so passing message overrides the generic copy while keeping Angular's semantic required. No new error key, no conceptual duplication.

Caveat: message applies to any error on the control. This field only validates required today, so it's fine — but if it later gained e.g. maxlength, it would show the same text for both.

Option B — extend the shared component to accept a per-key message override for built-in validators (a requiredErrorMessage input, mirroring the existing patternErrorMessage). This is the proper fix at the root, but it's a wider change since it touches all consumers of dot-field-validation-message.

My preference is Option A here: it gets the same field-specific copy through an API the component already offers, without adding an error key that overlaps with required.

…ge override

Replaces the custom commentRequired error key with Angular's standard
required, avoiding a semantic duplicate. Adds a requiredErrorMessage input
to the shared dot-field-validation-message component (mirroring the existing
patternErrorMessage) so a field can override just the required-error copy
without affecting other validators on the same control.

Addresses review feedback: #36464 (comment)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jcastro-dotcms

Copy link
Copy Markdown
Member Author

Good catch, thanks @nicobytes. Went with Option B (extend the shared component) rather than Option A as originally described — the literal snippet in Option A binds message statically to always show our copy, but getErrors() returns defaultMessage unconditionally whenever any error is present on the control (return this.defaultMessage ? this.defaultMessage : ...). Since this field's max-length validator returns { invalid: true } (not a string), once a comment exceeds 500 chars while non-empty, dirty && !valid is true and the same "A message is required." text would render — which is wrong for that case. A conditional message binding avoids that, but introduces a timing issue: errorMsg is recomputed synchronously off the field input's statusChanges subscription inside setErrors(), which fires before Angular's own change-detection cycle re-evaluates the conditional message expression — so the override value is effectively one tick stale.

Implemented in dd0f0fa:

  • Reverted to Angular's built-in required key (this.commentControl.setErrors({ required: true })) — no more semantic duplication.
  • Added a requiredErrorMessage input to DotFieldValidationMessageComponent, mirroring the existing patternErrorMessage pattern — it only overrides the copy for a required error, leaving every other validator's message (and every other consumer of the component) untouched. It's a static value bound once, so it doesn't hit the timing issue above.
  • Added test coverage in dot-field-validation-message.component.spec.ts for the new input, including a regression test confirming it doesn't leak into a pattern error on the same field.

All 45 tests across the two specs pass. Let me know if you'd rather see this scoped differently.

🤖 Generated with Claude Code

jcastro-dotcms and others added 2 commits July 8, 2026 11:36
…p textarea shift

activities-footer is `sticky bottom-0`, so when dot-field-validation-message
conditionally mounted/unmounted its <small> element, the footer's height
changed and, being bottom-anchored, pushed the comment textarea upward.
Wraps the validation message in an always-present, fixed-height slot so the
footer's height stays constant whether or not the error is showing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…f github.com:dotCMS/core into issue-36372-comment-section-button-styles-validation
@jcastro-dotcms

jcastro-dotcms commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Fixed the textarea shift (commit `c091148`).

Root cause: `activities-footer` is `sticky bottom-0`. `dot-field-validation-message` conditionally mounts/unmounts its `` internally (`@if`), so the row's height changed whenever the error appeared or cleared — and because the footer is bottom-anchored, that height change pushed the textarea upward instead of just growing/shrinking downward.

Rather than changing the shared component's internal `@if` to always render (which would add persistent reserved space to all ~25 other usages of `dot-field-validation-message` across the app — login, content types, personas, push-publish, etc. — none of which asked for that), I scoped the fix to this component: wrapped the validation message in a div.h-3.5 slot (data-testid="activities-error-slot") that's always present with a fixed height, whether or not the error is showing. The footer's total height is now constant across all states, so there's no more movement.

Added a regression test asserting the slot exists and keeps its height class through the no-error → error → cleared cycle.

🤖 Generated with Claude Code

@mergify

mergify Bot commented Jul 8, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@jcastro-dotcms
jcastro-dotcms added this pull request to the merge queue Jul 9, 2026
Merged via the queue into main with commit e6baf22 Jul 9, 2026
40 checks passed
@jcastro-dotcms
jcastro-dotcms deleted the issue-36372-comment-section-button-styles-validation branch July 9, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Comment section: primary Send / tertiary Clear buttons with empty-message validation (Edit Contentlet)

3 participants