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.

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

fix(datatable): fix sort parameters #356

Merged
merged 1 commit into from
Oct 24, 2020
Merged

fix(datatable): fix sort parameters #356

merged 1 commit into from
Oct 24, 2020

Conversation

albertms10
Copy link
Contributor

fixes #355


  • fix(datatable): fix sort parameters

@albertms10 albertms10 requested a review from metonym as a code owner October 23, 2020 14:09
@vercel
Copy link

vercel bot commented Oct 23, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-svelte/carbon-components-svelte/4xa4r9s43
✅ Preview: https://carbon-components-svelte-git-datatable-sort-fix.carbon-svelte.vercel.app

@metonym
Copy link
Collaborator

metonym commented Oct 23, 2020

@albertms10 Could you add an example that adapts the existing "Sortable" example that demonstrates the custom sort method?

@albertms10
Copy link
Contributor Author

albertms10 commented Oct 24, 2020

Something like using dayjs and i18n related dependencies:

<script lang="ts">
  import { DataTable } from "carbon-components-svelte";
  import dayjs from "dayjs";
  import relativeTime from "dayjs/plugin/relativeTime";
  import { numberfrom "svelte-i18n";

  dayjs.extend(relativeTime); // for `dayjs().fromNow()` method

  const headers = [
    {
      key: "name",
      value: "Name",
    },
    {
      key: "quantity",
      value: "Quantity",
      display: (quantity: number) => $number(quantity, { format: "EUR" }),
    },
    {
      key: "date",
      value: "Date",
      display: (date: string) => dayjs(date).fromNow(),
      sort: (a: string, b: string) => (dayjs(a).isBefore(dayjs(b)) ? -1 : 1),
    },
  ];

  const rows = [
    {
      id: 1,
      name: "John",
      quantity: 157.89,
      date: "2020-10-21",
    }
  ];
</script>

<DataTable {headers} {rows} />

If you find it fine, may I add it to the docs?

P.S.: It would be great if the display and sort method’s parameter types were automatically inferred, somehow.
It seems tricky, as having both rows and headers types related creates a circular dependency.

// Typing `headers` with `rows` objects data
const headers = [
  ...
  {
    key: "quantity",
    value: "Quantity",
    display: (quantity: rows[number]["quantity"]) => $number(quantity, { format: "EUR" }),
  },
  {
    key: "date",
    value: "Date",
    display: (date: rows[number]["date"]) => dayjs(date).fromNow(),
    sort: (a: rows[number]["date"], b: rows[number]["date"]) => (dayjs(a).isBefore(dayjs(b)) ? -1 : 1),
  },
];

But, in order to force rows array to have objects with a set of keys:

// Typing `rows` with a `headersList` array
const headersList = ["name", "quantity", "date"] as const;

type DataTableRows = {
  [P in typeof headersList[number] | "id"]: string;
};

const rows: DataTableRows = [
  {
    id: 1,
    nam: "John", // Will trigger a TS error
    quantity: 157.89,
    date: "2020-10-21",
  },
];

@metonym
Copy link
Collaborator

metonym commented Oct 24, 2020

To avoid adding additional dependencies, can you simplify the example even further to omit dayjs, svelte-i18n?

@albertms10
Copy link
Contributor Author

albertms10 commented Oct 24, 2020

Ok, what about:

<script lang="ts">
  import { DataTable } from "carbon-components-svelte";

  const headers = [
    {
      key: "name",
      value: "Name",
    },
    {
      key: "quantity",
      value: "Quantity",
      display: (quantity: number) => `${quantity} €`,
    },
    {
      key: "date",
      value: "Date",
      display: (date: string) => new Date(date).toLocaleString(),
      sort: (a: string, b: string) => new Date(a) - new Date(b),
    },
  ];

  const rows = [
    {
      id: 1,
      name: "John",
      quantity: 157.89,
      date: "2020-10-21",
    }
  ];
</script>

<DataTable {headers} {rows} />

@metonym
Copy link
Collaborator

metonym commented Oct 24, 2020

That works

@albertms10
Copy link
Contributor Author

albertms10 commented Oct 24, 2020

What do you think, @metonym, about the types that involve rows and headers? P.S. in #356 (comment)

@metonym
Copy link
Collaborator

metonym commented Oct 24, 2020

@albertms10 For the TypeScript comment, I'm not sure how it would be automatically inferred without typing it. Would typing headers as a tuple work?

Something like:

const headers: [{key: "quantity", value: "Quantity", display: (quantity: number) => string;}] = [{
  key: "quantity",
  value: "Quantity",
  display: (quantity: number) => `${quantity} €`,
}]

@albertms10
Copy link
Contributor Author

albertms10 commented Oct 24, 2020

To get things properly organized, I opened a new issue (#364) to talk about TypeScript type definitions and let this PR merge the way it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix DataTable sort method direction and key values
2 participants