1076 task list add scrollbar and fade to parent tasks and fix progress badge#23008
Conversation
Pull Request Test Coverage Report for Build 096ea5ca1d0dd515124a3d431dea93a8a2190b4aDetails
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR updates the task list UI to improve the task modal’s usability when content overflows, and adjusts the TasksProgressBadge markup to avoid nested-button console errors while keeping the badge clickable where needed.
Changes:
- Make
TasksProgressBadgerender as a non-button element by default, with anasprop for rendering as a<button>when needed. - Update the task modal content container to allow vertical scrolling and add a bottom fade overlay.
- Update/extend unit tests and snapshots to reflect the new rendered markup.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dashboard-frontend/src/task-list/components/tasks-progress-badge.js | Switch default root element to span and add as prop support for rendering as button. |
| packages/dashboard-frontend/src/task-list/components/task-modal.js | Enable scrollable modal content area and add a sticky gradient fade; render the “parent task” badge as a button. |
| packages/dashboard-frontend/tests/task-list/tasks-progress-badge.test.js | Add tests around the new as behavior and click handling. |
| packages/dashboard-frontend/tests/task-list/snapshots/tasks-progress-badge.test.js.snap | Snapshot updates for default span rendering and new “button” snapshot. |
| packages/dashboard-frontend/tests/task-list/snapshots/task-row.test.js.snap | Snapshot updates reflecting progress badge no longer rendering as a button in rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, [ onClick, parentTaskId ] ); | ||
|
|
||
| return <button onClick={ handleClick } disabled={ ! parentTaskId } className={ classNames( "yst-max-w-80 sm:yst-max-w-full yst-min-w-0 yst-truncate", className ) }> | ||
| return <Component onClick={ handleClick } className={ classNames( "yst-max-w-80 sm:yst-max-w-full yst-min-w-0 yst-truncate", className ) }> |
There was a problem hiding this comment.
The root element always gets onClick={ handleClick }, and handleClick always calls event.preventDefault() even when onClick/parentTaskId aren’t provided (so the badge is effectively non-actionable). This creates an unnecessary click handler on the default <span> and marks events as defaultPrevented for no reason. Consider only attaching the click handler (and calling preventDefault) when parentTaskId && onClick is truthy.
There was a problem hiding this comment.
I decided to remove the onClick if it's not a button and disable the button if either onClick, parentId or is loading.
I also removed the prevent default because this is not is a form.
| return <Component onClick={ handleClick } className={ classNames( "yst-max-w-80 sm:yst-max-w-full yst-min-w-0 yst-truncate", className ) }> | ||
| <Badge size="large" className="yst-bg-white yst-border yst-border-slate-200 yst-ps-1.5 yst-pe-2 yst-shadow-sm yst-h-6 yst-w-full"> |
There was a problem hiding this comment.
When rendering the badge as a <button> (via as="button"), the component doesn’t set type="button", so it may submit a surrounding form by default. Also consider setting disabled/aria-disabled when onClick or parentTaskId is missing to avoid focusable UI that does nothing.
There was a problem hiding this comment.
I added a type prop.
| const { container } = render( | ||
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } onClick={ onClick } parentTaskId="task-1" /> | ||
| ); | ||
| fireEvent.click( container.firstChild ); | ||
| expect( onClick ).toHaveBeenCalledTimes( 1 ); | ||
| expect( onClick ).toHaveBeenCalledWith( "task-1" ); | ||
| } ); | ||
|
|
||
| it( "does not call onClick when parentTaskId is missing", () => { | ||
| const onClick = jest.fn(); | ||
| const { container } = render( | ||
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } onClick={ onClick } /> | ||
| ); | ||
| fireEvent.click( container.firstChild ); | ||
| expect( onClick ).not.toHaveBeenCalled(); | ||
| } ); | ||
|
|
||
| it( "does not call onClick when onClick is missing", () => { | ||
| const { container } = render( | ||
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } parentTaskId="task-1" /> | ||
| ); | ||
| // Should not throw when clicked without an onClick handler. | ||
| expect( () => fireEvent.click( container.firstChild ) ).not.toThrow(); | ||
| } ); | ||
|
|
||
| it( "calls event.preventDefault on click", () => { | ||
| const { container } = render( | ||
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } onClick={ jest.fn() } parentTaskId="task-1" /> | ||
| ); | ||
| const event = new MouseEvent( "click", { bubbles: true, cancelable: true } ); | ||
| jest.spyOn( event, "preventDefault" ); | ||
| fireEvent( container.firstChild, event ); |
There was a problem hiding this comment.
The click-behavior test renders TasksProgressBadge with onClick/parentTaskId but doesn’t set as="button", so it’s validating click behavior on a non-interactive default element. If the intent is “clickable badge should be a button for accessibility”, update this test to render with as="button" and preferably query/click via getByRole("button").
| const { container } = render( | |
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } onClick={ onClick } parentTaskId="task-1" /> | |
| ); | |
| fireEvent.click( container.firstChild ); | |
| expect( onClick ).toHaveBeenCalledTimes( 1 ); | |
| expect( onClick ).toHaveBeenCalledWith( "task-1" ); | |
| } ); | |
| it( "does not call onClick when parentTaskId is missing", () => { | |
| const onClick = jest.fn(); | |
| const { container } = render( | |
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } onClick={ onClick } /> | |
| ); | |
| fireEvent.click( container.firstChild ); | |
| expect( onClick ).not.toHaveBeenCalled(); | |
| } ); | |
| it( "does not call onClick when onClick is missing", () => { | |
| const { container } = render( | |
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } parentTaskId="task-1" /> | |
| ); | |
| // Should not throw when clicked without an onClick handler. | |
| expect( () => fireEvent.click( container.firstChild ) ).not.toThrow(); | |
| } ); | |
| it( "calls event.preventDefault on click", () => { | |
| const { container } = render( | |
| <TasksProgressBadge completedTasks={ 2 } totalTasks={ 5 } onClick={ jest.fn() } parentTaskId="task-1" /> | |
| ); | |
| const event = new MouseEvent( "click", { bubbles: true, cancelable: true } ); | |
| jest.spyOn( event, "preventDefault" ); | |
| fireEvent( container.firstChild, event ); | |
| render( | |
| <TasksProgressBadge | |
| completedTasks={ 2 } | |
| totalTasks={ 5 } | |
| as="button" | |
| onClick={ onClick } | |
| parentTaskId="task-1" | |
| /> | |
| ); | |
| fireEvent.click( screen.getByRole( "button" ) ); | |
| expect( onClick ).toHaveBeenCalledTimes( 1 ); | |
| expect( onClick ).toHaveBeenCalledWith( "task-1" ); | |
| } ); | |
| it( "does not call onClick when parentTaskId is missing", () => { | |
| const onClick = jest.fn(); | |
| render( | |
| <TasksProgressBadge | |
| completedTasks={ 2 } | |
| totalTasks={ 5 } | |
| as="button" | |
| onClick={ onClick } | |
| /> | |
| ); | |
| fireEvent.click( screen.getByRole( "button" ) ); | |
| expect( onClick ).not.toHaveBeenCalled(); | |
| } ); | |
| it( "does not call onClick when onClick is missing", () => { | |
| render( | |
| <TasksProgressBadge | |
| completedTasks={ 2 } | |
| totalTasks={ 5 } | |
| as="button" | |
| parentTaskId="task-1" | |
| /> | |
| ); | |
| // Should not throw when clicked without an onClick handler. | |
| expect( () => fireEvent.click( screen.getByRole( "button" ) ) ).not.toThrow(); | |
| } ); | |
| it( "calls event.preventDefault on click", () => { | |
| render( | |
| <TasksProgressBadge | |
| completedTasks={ 2 } | |
| totalTasks={ 5 } | |
| as="button" | |
| onClick={ jest.fn() } | |
| parentTaskId="task-1" | |
| /> | |
| ); | |
| const event = new MouseEvent( "click", { bubbles: true, cancelable: true } ); | |
| jest.spyOn( event, "preventDefault" ); | |
| fireEvent( screen.getByRole( "button" ), event ); |
There was a problem hiding this comment.
I removed the onclick if it's not a button.
|
CR ✅ + AT ✅ |
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.[yoast-doc-extension], added test instructions for Yoast SEO for Google Docs and attached theGoogle Docs Add-onlabel to this PR.Documentation
Quality assurance
grunt build:imagesand commited the results, if my PR introduces new images or SVGs.Innovation
innovationlabel.Fixes https://github.com/Yoast/reserved-tasks/issues/1076