Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 20 commits into from
Sep 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7bedf1e
adjust size, spaing, and position of datalistactions
chris-at-jobber Jun 4, 2024
f155370
Merge branch 'master' into poc-moving-datalist-actions-around
chris-at-jobber Jul 10, 2024
f2d66e3
Merge branch 'master' into poc-moving-datalist-actions-around
chris-at-jobber Sep 3, 2024
65bcba2
prefer bottom positioning for action tooltips
chris-at-jobber Sep 3, 2024
a1d95a3
update Tooltip animations
chris-at-jobber Sep 3, 2024
1dd63a5
change how datalistitemaction menu is positioned
chris-at-jobber Sep 3, 2024
51be5c7
adjustments
chris-at-jobber Sep 4, 2024
6997e47
Merge branch 'master' into poc-moving-datalist-actions-around
chris-at-jobber Sep 17, 2024
e3f6ddd
just a smidge of space
chris-at-jobber Sep 17, 2024
1fe979b
remove usage of OverflowFade from actions
chris-at-jobber Sep 18, 2024
a124cd4
remove some leftover styling
chris-at-jobber Sep 18, 2024
17d487e
fix test
chris-at-jobber Sep 18, 2024
842e037
Merge branch 'master' into poc-moving-datalist-actions-around
chris-at-jobber Sep 18, 2024
6c8349c
remove negative var
chris-at-jobber Sep 18, 2024
872d90a
Merge branch 'poc-moving-datalist-actions-around' of https://github.c…
chris-at-jobber Sep 18, 2024
b610fba
Fix prettier error in DataListBulkActions
taylorvnoj Sep 18, 2024
6945243
Merge branch 'master' into poc-moving-datalist-actions-around
taylorvnoj Sep 18, 2024
286f1c2
use a nicer bulk test
chris-at-jobber Sep 18, 2024
31f6dc7
Merge branch 'poc-moving-datalist-actions-around' of https://github.c…
chris-at-jobber Sep 18, 2024
7794a79
fix import orders
chris-at-jobber Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions packages/components/src/DataList/components/DataListActions/DataListActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import React, { Children, ReactElement, isValidElement } from "react";
import { Tooltip } from "@jobber/components/Tooltip";
import { Button } from "@jobber/components/Button";
import { DataListOverflowFade } from "../DataListOverflowFade";
import {
DataListActionProps,
DataListActionsProps,
Expand All @@ -25,7 +24,7 @@ export function DataListActions<T extends DataListObject>({
childrenArray.splice(0, exposedActions.length);

return (
<DataListOverflowFade>
<span data-testid="ATL-DataListActions">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{exposedActions.map(({ props }) => {
if (props.visible && !props.visible(activeItem)) return null;
if (!props.icon) return null;
Expand Down Expand Up @@ -54,14 +53,14 @@ export function DataListActions<T extends DataListObject>({
(props.onClick as () => void)?.();
}
}}
type="secondary"
type="tertiary"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tertiary button in the "individual" actions

variation="subtle"
/>
</Tooltip>
);
})}

<DataListItemActionsOverflow actions={childrenArray} />
</DataListOverflowFade>
</span>
);
}
4 changes: 1 addition & 3 deletions packages/components/src/DataList/components/DataListBulkActions/DataListBulkActions.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ afterAll(() => {
describe("DataListBulkActions", () => {
it("should render the 4 buttons", () => {
renderComponent();
const overflowContainer = screen.getByTestId(
"ATL-DataListFilters-Container",
);
const overflowContainer = screen.getByTestId("ATL-DataListActions");

expect(within(overflowContainer).getAllByRole("button")).toHaveLength(4);
});
Expand Down
5 changes: 2 additions & 3 deletions packages/components/src/DataList/components/DataListItemActions/DataListItemActions.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
.menu {
display: flex;
position: absolute;
top: calc(var(--space-small) * -1);
top: -26px;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this number is relatively magic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪄 I trust in your magic, Chris.

right: 0;
z-index: var(--elevation-menu);
box-shadow: var(--shadow-base);
padding: var(--space-smaller);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since padding is now 0, no need to declare it

border: var(--border-base) solid var(--color-border);
border-radius: var(--radius-base);
background-color: var(--color-surface);
gap: var(--space-smaller);
gap: var(--space-smallest);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
4 changes: 2 additions & 2 deletions packages/components/src/DataList/components/DataListItemActions/DataListItemActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ export function DataListItemActions<T extends DataListObject>(
}

const variants: Variants = {
hidden: { opacity: 0, y: 10 },
visible: { opacity: 1, y: 0 },
hidden: { opacity: 0, y: 0 },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusts the animation to map better to the new position

visible: { opacity: 1, y: 4 },
};

interface InternalDataListItemActionsProps<T extends DataListObject> {
Expand Down
2 changes: 1 addition & 1 deletion ...nents/src/DataList/components/DataListItemActionsOverflow/DataListItemActionsOverflow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function DataListItemActionsOverflow<T extends DataListObject>({
<Button
icon="more"
ariaLabel="More actions"
type="secondary"
type="tertiary"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

variation="subtle"
onClick={handleMoreClick}
/>
Expand Down
9 changes: 4 additions & 5 deletions packages/components/src/DataList/components/DataListOverflowFade/DataListOverflowFade.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
.fadeContainer {
--overflow-fade--offset: var(--space-smaller);
--overflow-fade--negative-offset: calc(var(--overflow-fade--offset) * -1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

--overflow-fade--offset: 0;

align-self: center;
position: relative;
Expand All @@ -20,7 +19,7 @@
}

.overflowGrid {
margin: var(--overflow-fade--negative-offset);
margin: var(--overflow-fade--offset);
padding: var(--overflow-fade--offset);
overflow-x: auto;
overflow-y: visible;
Expand All @@ -46,10 +45,10 @@
}

.overflowLeft::before {
left: var(--overflow-fade--negative-offset);
left: var(--overflow-fade--offset);
}

.overflowRight::after {
--data-list-overflow-shadow-angle: to left;
right: var(--overflow-fade--negative-offset);
right: var(--overflow-fade--offset);
}
Loading