feat(components): improve positioning of DataList actions [JOB-99926]

[chris-at-jobber]

Motivations

When the content in the last column of a DataList row is full-width or right-aligned, the "hover" actions on the row can be obscured. This violates the "recognition over recall" principle.

Changes

Changed

• reduced the size of the DataList hover actions and changed their offset to avoid covering contents of the rightmost column

Testing

Hover over a row

Changes can be
tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[github-actions]

Could not publish Pre-release for 6945243. View Logs
The problem is likely in the NPM Publish or NPM CI step in the Trigger Pre-release Build Job.

Previous build information:

Published Pre-release for 6945243 with versions:

  - @jobber/components@5.7.9-poc-moving-6945243.15+69452437

To install the new version(s) for Web run:

npm install @jobber/components@5.7.9-poc-moving-6945243.15+69452437
```.

[cloudflare-workers-and-pages]

Deploying atlantis with    Cloudflare Pages

Latest commit:	7794a79
Status:	✅  Deploy successful!
Preview URL:	https://4d291804.atlantis.pages.dev
Branch Preview URL:	https://poc-moving-datalist-actions.atlantis.pages.dev

View logs

[chris-at-jobber]

Adjusts the animation to map better to the new position

[chris-at-jobber]

Tertiary button in the "individual" actions

[chris-at-jobber]

tertiary button in the "more" actions

unclear to me at this point why this action is different from the DataList actions but here we are

[taylorvnoj]

It's a good separation of concerns.
DataListActions are rendering the primary actions we see on hover. DataListItemActionsOverflow is that ••• menu button and needs to handle state for opening/closing the menu

I think we could also pretty easily extract & simplify DataListItemActionsOverflow to share with other menus that would have the same behaviour. Obviously not something we'll do now but, hope that helps clear things a little

[chris-at-jobber]

This impacts both the "row" actions and the "bulk" actions, but has no negative impact on either instance:

[chris-at-jobber]

At one point the design direction was to have the bulk actions be scrollable, which is likely why this was implemented, but you can see that they collapse down to the ••• menu once the viewport is narrowed so the gradient fade support added by DataListOverFlowFade was not giving us anything, and was in turn:

• coupling the spacing of these actions to the spacing in the filters bar
• clipping the focus shadow of the actions

So two birds with one stone, as they say

[chris-at-jobber]

Yes this number is relatively magic

[taylorvnoj]

🪄 I trust in your magic, Chris.

[chris-at-jobber]

DataListOverFlowFade came with a testid that was being referenced in BatchActions test - this preserves the test

[taylorvnoj]

hmmm, we have a file, packages/components/src/DataList/components/DataListBulkActions/DataListBulkActions.const.ts that holds

export const BULK_ACTIONS_CONTAINER_TEST_ID =
  "ATL-DataListBulkActions-Container";

Which actually isn't being used anywhere. We should use BULK_ACTIONS_CONTAINER_TEST_ID instead, but not in this file.

We should:

• remove the span in DataListActions.tsx, it just becomes empty fragment
• DataListBulkActions.tsx would now have the bulk actions test id:

import React from "react";
import styles from "./DataListBulkActions.css";
import { BULK_ACTIONS_CONTAINER_TEST_ID } from "./DataListBulkActions.const";
import { DataListBulkActionsProps } from "../../DataList.types";
import { useDataListContext } from "../../context/DataListContext";
import { DataListActions } from "../DataListActions";
import { useResponsiveSizing } from "../../hooks/useResponsiveSizing";

// This component is meant to capture the props of the DataList.BulkActions
export function DataListBulkActions(
  // eslint-disable-next-line @typescript-eslint/no-unused-vars
  _: DataListBulkActionsProps,
) {
  return null;
}

export function InternalDataListBulkActions() {
  const { bulkActionsComponent } = useDataListContext();
  if (!bulkActionsComponent) return null;

  const { children } = bulkActionsComponent.props;

  const { sm } = useResponsiveSizing();

  // Collapse all actions under "More actions" when breakpoint is smaller than sm
  const itemsToExpose = sm ? 3 : 0;

  return (
    <div
      className={styles.bulkActions}
      data-testid={BULK_ACTIONS_CONTAINER_TEST_ID}
    >
      <DataListActions itemsToExpose={itemsToExpose}>
        {children}
      </DataListActions>
    </div>
  );
}

• then in DataListBulkActions.test.tsx you put that testid in there instead:

import { BULK_ACTIONS_CONTAINER_TEST_ID } from "./DataListBulkActions.const";

describe("DataListBulkActions", () => {
  it("should render the 4 buttons", () => {
    renderComponent();
    const overflowContainer = screen.getByTestId(
      BULK_ACTIONS_CONTAINER_TEST_ID,
    );

    expect(within(overflowContainer).getAllByRole("button")).toHaveLength(4);
  });

This way, we're using an unused testid, which is more correct to what we're testing. Let me know if I'm not communicating this well enough we can hop on a call

[chris-at-jobber]

Since padding is now 0, no need to declare it

[chris-at-jobber]

Keeps a pinch of space between the actions so the focus states don't encroach too heavily on sibling buttons

[chris-at-jobber]

These values are now the same and can be applied accordingly with one variable

I considered removing the margin and padding declarations, but since the positions of the gradients are coupled to those values, felt worth keeping them in sync

[taylorvnoj]

Looking really really good - we're no longer blocking any info 👌

Just that one change with the testid and this will be good to go!

[taylorvnoj]

@chris-at-jobber for the failed lint_javascript, to see the errors, run:

npm run lint:js 

These two have to be fixed:

[taylorvnoj]

🎉 thanks for fixing the test! SHIP IT