Fixes #33112: restore delete, hover affordance and expand in task com… - #33178
Fixes #33112: restore delete, hover affordance and expand in task com…#33178Vansh0310 wants to merge 9 commits into
Conversation
…d in task comments
Incident task comments had no way to delete a comment, no visible affordance
for the action, and long comments were clamped with no way to expand them.
- Drop enableSeeMoreVariant={false} on the comment previewer so it falls back
to its default of true and long comments get a working view-more toggle.
- Reveal a delete action on hover for the comment author or an admin, wired to
deleteTaskComment through the shared DeleteModal. It is positioned out of
flow so showing it cannot reflow the comment body.
- Pass currentUser down from TaskTabNew for the permission check, and refetch
the thread via onCommentDeleted so the list reflects the delete.
Adds TaskCommentCard.test.tsx covering rendering, both regression guards, the
four permission combinations, hover behaviour and the delete flow including the
failure path. Extends the TaskTabNew suite to confirm the two new props are
wired through.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
The delete affordance was an Ant Design Icon with an onClick, mounted only while the mouse was over the card. It had no button role, no tab stop and no accessible name, so keyboard and screen-reader users could never reach it. - Replace it with ButtonUtility from ui-core-components, which renders a real button via react-aria and takes an accessible name from its tooltip. - Keep it mounted whenever the user may delete, and reveal it with CSS on card hover or on its own focus instead of a mouse-only hover state. This drops the isHovered state and the wrapper's role="presentation". - Move the inline positioning styles onto Tailwind utilities. Covered by tests asserting the button role and accessible name, that it is focusable without a hover, and that Tab followed by Enter opens the modal.
… into fix/incident-comment-feed-33112
Main added a rule that icons come from @openmetadata/ui-core-components/icons and must not be imported directly from assets/ paths, since bypassing the re-export can diverge on version upgrades. Swap the raw ic-delete.svg import for the Delete icon the package exports.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
anuj-kumary
added
the
safe to test
Jest test CoverageUI tests summary
|
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 5 resolved / 5 findingsRestores delete, hover affordance, and expand functionality in task comments. Delete icon now appears on hover for comment authors and admins, with a confirmation dialog. Also fixes two pre-existing bugs: hover no longer causes text reflow, and long comments now show a working "View More"/"View Less" toggle instead of being silently truncated. All findings addressed: delete action is now keyboard and screen-reader accessible, the delete icon uses core-components, DeleteModal z-index is scoped to avoid affecting other components, modal mounting is conditional, and container mutations are avoided. ✅ 5 resolved✅ Quality: Delete affordance is not keyboard/screen-reader accessible
✅ Quality: New delete widget uses Ant Design Icon instead of core-components
✅ Quality: Global DeleteModal z-index bump affects all 75 consumers
✅ Quality: DeleteModal mounted for every comment even when canDelete is false
✅ Quality: Parent tabindex fallback permanently mutates shared container
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
anuj-kumary
added
the
safe to test
❌ UI Checkstyle Failed❌ Playwright - Guardrails + ESLint + Prettier + Organise ImportsEither a Playwright test file has linting/formatting issues, or a guardrail check failed: ESLint rule unit tests, a new guardrail violation, a stale suppression entry (its violation was fixed but the baseline was not pruned), a blanket Affected filesSubtest: const a = 'server.entity-fetch-error';\n const b = 'server.entity-fetch-error';\n const c = 'server.entity-fetch-error';not ok 27 - the suppressions baseline matches its recorded state exactly fail 1error Command failed with exit code 1. 🔍 ESLint findings in this PR's files — 0 error(s), 27 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 27 warning(s) across 2 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
|
3 participants
Describe your changes:
Fixes #33112
Adds the ability to delete an individual task comment. The comment's author or an
admin can now delete a comment via a trash icon that appears on hover next to the
comment, with a confirmation dialog before the delete goes through.
Also fixes two pre-existing bugs in the comment thread surfaced while building this:
full-width
Space, which had no room for it — hovering over any comment causedthe comment text to visibly reflow/shrink and then snap back. The icon is now
absolutely positioned over the content (matching the existing pattern in
CommentCard.component.tsx/ActivityFeedActions.tsx), so hovering no longermoves anything.
RichTextEditorPreviewNewclamps text to ~2 lines regardless of theenableSeeMoreVariantprop — that prop only controls whether the "ViewMore"/"View Less" toggle renders, not whether the clamp applies. An incorrect
enableSeeMoreVariant={false}on this component suppressed the toggle while theclamp stayed active. Removed it so the toggle now appears whenever a comment
overflows.
Type of change:
High-level design:
TaskCommentCardnow acceptscurrentUserand anonCommentDeletedcallback fromTaskTabNew. AcanDeletecheck (author match ORcurrentUser.isAdmin) gates ahover-only delete icon; clicking it opens the shared
DeleteModal, which on confirmcalls
deleteTaskComment(taskId, commentId)and, on success, callsonCommentDeleted(wired tofetchUpdatedThread(task.id, true)inTaskTabNewtorefresh the thread) and closes the modal. Failures show an error toast and leave the
modal open so the user can retry.
Tests:
Use cases covered
being silently truncated.
Unit tests
TaskCommentCard.component.test.tsx(new),TaskTabNew.component.test.tsx(extended to cover thecurrentUser/onCommentDeletedwiring).Backend integration tests
DELETE .../tasks/{taskId}/comments/{commentId}endpoint anddeleteTaskCommentclient function, both already in place).
Ingestion integration tests
Playwright (UI) tests
comment removed from the thread.
Manual testing performed
delete icon appears on hover only for my own comments, and deleting removes the
comment and refreshes the thread.
regardless of author.
comments authored by others.
the text cutting off.
UI screen recording / screenshots:
[attach before/after — the reflow-on-hover and the cut-off-text cases are worth
showing explicitly since they're easy to miss in a still screenshot]
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.