Adds Pagination to Data Table #1367

joshri · 2022-01-28T22:56:15Z

Closes: #1361

Adds pagination functionality to the data table + unit tests

tested in react storybook npm run storybook

… table

…wrong so I fixed it

jpellizzari

One thing we need to keep in mind is what pagination is used for on the back end; we use pagination to show a subset of all rows in the database.

When the controls are clicked, we need to fetch the next page from the back-end, then show that data in the table.

Instead of paging in the front end, we can simplify the logic a bit to just have click handlers and let the component that wraps the <DataTable /> figure out how to fetch the page.

Let me know if this makes sense or if you want to discuss on a call.

ui/components/CommitsTable.tsx

ui/components/DataTable.tsx

jpellizzari

I think I see a way to make this API a little nicer. What about:

function MyComponent() {
  const handleForward = () => {
    console.log("forward!");
  };

  const handleBack = () => {
    console.log("back");
  };
  return (
    <DataTable rows={[]} fields={[]}>
      <PaginationControls onForward={handleForward} onBack={handleBack} />
    </DataTable>
  );
}

Or, if you don't like having the controls as children, what about as a prop:

function MyComponent() {
  const handleForward = () => {
    console.log("forward!");
  };

  const handleBack = () => {
    console.log("back");
  };
  return (
    <DataTable
      rows={[]}
      fields={[]}
      paginationControls={
        <PaginationControls onForward={handleForward} onBack={handleBack} />
      }
    />
  );
}

This lifts the controls up and you avoid prop drilling.

jpellizzari · 2022-01-31T19:53:22Z

ui/components/ConditionsTable.tsx

-        { label: "Status", value: "status" },
-        { label: "Reason", value: "reason" },
-        { label: "Message", value: "message" },
+        { label: "type", displayLabel: "Type", value: "type" },


There shouldn't be a label and displayLabel prop here; label is already mean to give control over what gets displayed.

For the _.sort function to work the labels have to be all lowercase to match the fields on the rows which meant of lot of string.toLowerCase()-ing which I h8ed.

I would go with string.toLowerCase() myself. As it is in this PR, it is a leaky abstraction, and you are asking the user to think about how the sorting logic works.

The only other thing that's coming to mind is if someone wants to sort columns with a label that won't match when it's shifted to lowercase. Like "Last Commit" with last_commit as the key. It makes those columns not sortable right?

You can do stuff like _.lowerCase(_.snakeCase(string)) if you want to normalize the values. I would have to see the full filtering implementation to say for sure.

ui/components/DataTable.tsx

ui/components/Pagination.tsx

bia · 2022-02-01T13:30:24Z

It's beautiful!

Some design-system nits (that @mmoulian and I will prepare better for you in the future)

All of the text should be 14px instead of 16pxand 20px, with the title weight semi-bold (not bold)
The heading/title which is sorting the table (with the arrow) should change color to Application/text/dark or #1a1a1a
could 'No rows' be renamed 'No data'? We're thinking something like this :

(icon svg [here](https://drive.google.com/file/d/1orX5jeBpj25Tepww_ITmcJodcQbZlcKp/view?usp=sharing) )

jpellizzari

Looks good, save for the blockers around displayLabel and <Link /> text sizes

jpellizzari · 2022-02-01T19:37:50Z

ui/components/ConditionsTable.tsx

-        { label: "Status", value: "status" },
-        { label: "Reason", value: "reason" },
-        { label: "Message", value: "message" },
+        { label: "type", displayLabel: "Type", value: "type" },


I would go with string.toLowerCase() myself. As it is in this PR, it is a leaky abstraction, and you are asking the user to think about how the sorting logic works.

jpellizzari · 2022-02-01T19:39:03Z

ui/components/Link.tsx

@@ -13,7 +13,11 @@ type Props = {
 };

 function Link({ children, href, className, to = "", newTab, ...props }: Props) {
-  const txt = <Text color="primary">{children}</Text>;
+  const txt = (
+    <Text size="small" color="primary">


We want to change all <Link /> components to have small text?

jpellizzari · 2022-02-01T19:39:24Z

ui/components/Pagination.tsx

+  /** func for skip to start button */
+  onSkipBack: () => void;
+  /** onChange func for perPage select */
+  onSelect: (value) => void;


Let's give this value a type. Is it a string? Number?

it's an event, and it needs to always be an event right? Is there a way to express that in Typescript?

Something like React.SyntheticEvent or some such:

Suggested change

onSelect: (value) => void;

onSelect: (ev: React.SyntheticEvent) => void;

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/14d95eb0fe90f5e0579c49df136cccdfe89b2855/types/react/index.d.ts#L1167

You can also just leave it blank if you don't expect the user to do anything with the arg.

jpellizzari · 2022-02-01T19:39:52Z

ui/components/Pagination.tsx

+          variant="text"
+          aria-label="skip to first page"
+          disabled={index === 0}
+          onClick={() => onSkipBack()}


Suggested change

onClick={() => onSkipBack()}

onClick={onSkipBack}

ui/components/__tests__/Pagination.test.tsx

jpellizzari · 2022-02-01T19:42:03Z

ui/components/__tests__/Pagination.test.tsx

+    expect(onSkipBack).toHaveBeenCalledTimes(1);
+    fireEvent.click(back);
+    expect(onBack).toHaveBeenCalledTimes(1);
+    expect(displayText).toBeTruthy();


What should this display text actually be? .toBeTruthy() will return ok on literally any string.

Yes but the test will fail if it does not match /6 - 6 out of 10/ because it won't find the string at all

jpellizzari

LGTM, save for the displayLabel thing

jpellizzari · 2022-02-01T20:04:29Z

ui/components/ConditionsTable.tsx

-        { label: "Status", value: "status" },
-        { label: "Reason", value: "reason" },
-        { label: "Message", value: "message" },
+        { label: "type", displayLabel: "Type", value: "type" },


You can do stuff like _.lowerCase(_.snakeCase(string)) if you want to normalize the values. I would have to see the full filtering implementation to say for sure.

jpellizzari · 2022-02-01T20:06:38Z

ui/components/Pagination.tsx

+  /** func for skip to start button */
+  onSkipBack: () => void;
+  /** onChange func for perPage select */
+  onSelect: (value) => void;


Something like React.SyntheticEvent or some such:

Suggested change

onSelect: (value) => void;

onSelect: (ev: React.SyntheticEvent) => void;

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/14d95eb0fe90f5e0579c49df136cccdfe89b2855/types/react/index.d.ts#L1167

You can also just leave it blank if you don't expect the user to do anything with the arg.

…nk size in data table + other small fixes

bia · 2022-02-02T14:06:16Z

ui/components/DataTable.tsx

    </div>
  );
 }

 export const DataTable = styled(UnstyledDataTable)`
  h2 {
+    font-size: 14px;
+    font-weight: 600;


I have played around with this in the inspector and for me this only renders bold (> 600) or regular (<600) – is semi-bold Proxima Nova not getting imported somewhere?

Hmmmm...I feel like I've seen that before, but I just tested it here's the screenshots:

font-weight 800:

font-weight 600:

It DEFINITELY changes but it is subtle for sure

* pagination added * added tests * added displayLabel key to fields to avoid manipulating string in data table * added display label field to data table story * display label fix in commits table * woke up in a cold sweat realizing that the indexing for the text was wrong so I fixed it * redo w/o tests * redo two with tests * data table styling changes * removed displayLabel and added textProps to Link in order to alter link size in data table + other small fixes * got rid of TextProps it was bad

joshri added 3 commits January 28, 2022 16:02

pagination added

6019b4a

added tests

27228eb

added displayLabel key to fields to avoid manipulating string in data…

cac66f9

… table

joshri added the area/ui Issues that require front-end work label Jan 28, 2022

joshri requested review from bia, JamWils and jpellizzari January 28, 2022 22:56

joshri added 3 commits January 28, 2022 17:56

added display label field to data table story

da01366

display label fix in commits table

5229ca7

woke up in a cold sweat realizing that the indexing for the text was …

3ff82f2

…wrong so I fixed it

jpellizzari requested a review from AlinaGoaga January 31, 2022 17:06

jpellizzari reviewed Jan 31, 2022

View reviewed changes

ui/components/CommitsTable.tsx Outdated Show resolved Hide resolved

ui/components/DataTable.tsx Outdated Show resolved Hide resolved

redo w/o tests

12401f9

jpellizzari reviewed Jan 31, 2022

View reviewed changes

ui/components/Pagination.tsx Outdated Show resolved Hide resolved

redo two with tests

d1377fa

data table styling changes

068d1d8

jpellizzari reviewed Feb 1, 2022

View reviewed changes

jpellizzari approved these changes Feb 1, 2022

View reviewed changes

joshri added 2 commits February 1, 2022 15:32

removed displayLabel and added textProps to Link in order to alter li…

aa51a5a

…nk size in data table + other small fixes

got rid of TextProps it was bad

aef334d

bia approved these changes Feb 2, 2022

View reviewed changes

joshri merged commit 0d6fd9c into main Feb 2, 2022

joshri deleted the 1361-pagination branch February 2, 2022 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Pagination to Data Table #1367

Adds Pagination to Data Table #1367

joshri commented Jan 28, 2022

jpellizzari left a comment

jpellizzari left a comment

jpellizzari Jan 31, 2022

joshri Jan 31, 2022

jpellizzari Feb 1, 2022

joshri Feb 1, 2022

jpellizzari Feb 1, 2022

bia commented Feb 1, 2022 •

edited

jpellizzari left a comment •

edited

jpellizzari Feb 1, 2022

jpellizzari Feb 1, 2022

jpellizzari Feb 1, 2022

joshri Feb 1, 2022

jpellizzari Feb 1, 2022

jpellizzari Feb 1, 2022

jpellizzari Feb 1, 2022

joshri Feb 1, 2022

jpellizzari left a comment

jpellizzari Feb 1, 2022

jpellizzari Feb 1, 2022

bia Feb 2, 2022

joshri Feb 2, 2022

joshri Feb 2, 2022

	onSelect: (value) => void;
	onSelect: (ev: React.SyntheticEvent) => void;

Adds Pagination to Data Table #1367

Adds Pagination to Data Table #1367

Conversation

joshri commented Jan 28, 2022

jpellizzari left a comment

Choose a reason for hiding this comment

jpellizzari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bia commented Feb 1, 2022 • edited

jpellizzari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpellizzari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bia commented Feb 1, 2022 •

edited

jpellizzari left a comment •

edited