Feature: refactor tables

a0m0rajab commented 1 year ago

Type of feature

🧑‍💻 Refactor

Current behavior

The tables of the contributors and the repositories page are the same, they have the same issues and problems, yet fixing one of them would need to have the same work done for the other, it would be great if the tables turned into a component and used as one component.

Additional context

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

Contributing Docs

[ ] I agree to follow this project's Contribution Docs

a0m0rajab commented 1 year ago

Since the tables are the same, if we detect an issue in one of them it would make it faster and easier to solve all issues if the tables were in one component.

bdougie commented 1 year ago

@a0m0rajab appreciate you opening the issue, but more context is needed. Could you linking the code and highlight the similarities. There is not enough context to remove the "needs triage" label or justify us working on this.

a0m0rajab commented 1 year ago

@bdougie I will go with how I noticed it:

When this PR made #1437 I noticed that: lg:min-w-[1150px] does not fit the tailwind design system, since large screen minimum width is 1024px this is why the code were removed in this line.

If we do a double check on the codebase, we can find the same issue at these lines:

https://github.com/open-sauced/insights/blob/31a9a6abda62c84475266e602ad95a5b62ab5fab/components/organisms/Contributors/contributors.tsx#L85-L89

Updating one table will require updating the other table to solve the issues in both of them.

Another type of similarity is the table title:

https://github.com/open-sauced/insights/blob/31a9a6abda62c84475266e602ad95a5b62ab5fab/components/organisms/Repositories/repositories.tsx#L121-L141

https://github.com/open-sauced/insights/blob/31a9a6abda62c84475266e602ad95a5b62ab5fab/components/organisms/RepositoriesTable/repositories-table.tsx#L51-L63

https://github.com/open-sauced/insights/blob/31a9a6abda62c84475266e602ad95a5b62ab5fab/components/molecules/ContributorListTableHeader/contributor-list-table-header.tsx#L20-L44

we are using the same logic for both of it.

Plus, while doing the refactor, it would need to change the

elements to native HTML tags <tr> and td for accessibility: https://www.w3.org/WAI/tutorials/tables/

This refactor might seem a bit complex right now, but it will add ease of use for the code for the next development or any issues would be found in the code.

I have in mind that we can have one table component where we pass the data to it and it will convert this data to table dynamically, creating the column/row design for it.

a0m0rajab commented 1 year ago

From other Design system we can check this from vue: where it has the logic of passing items as an array and using it: https://vuetifyjs.com/en/components/data-tables/basics/#item

Example code:

  headers: [
          {
            title: 'Dessert (100g serving)',
            align: 'start',
            sortable: false,
            key: 'name',
          },
          { title: 'Calories', key: 'calories' },
          { title: 'Fat (g)', key: 'fat' },
          { title: 'Carbs (g)', key: 'carbs' },
          { title: 'Protein (g)', key: 'protein' },
          { title: 'Iron (%)', key: 'iron' },
        ],
        desserts: [
          {
            name: 'Frozen Yogurt',
            calories: 159,
            fat: 6.0,
            carbs: 24,
            protein: 4.0,
            iron: '1%',
          },]

or ShadCN: https://ui.shadcn.com/docs/components/table where it uses the header as a separate code then populate the data

 <Table>
      <TableCaption>A list of your recent invoices.</TableCaption>
      <TableHeader>
        <TableRow>
          <TableHead className="w-[100px]">Invoice</TableHead>
          <TableHead>Status</TableHead>
          <TableHead>Method</TableHead>
          <TableHead className="text-right">Amount</TableHead>
        </TableRow>
      </TableHeader>
      <TableBody>
        {invoices.map((invoice) => (
          <TableRow key={invoice.invoice}>
            <TableCell className="font-medium">{invoice.invoice}</TableCell>
            <TableCell>{invoice.paymentStatus}</TableCell>
            <TableCell>{invoice.paymentMethod}</TableCell>
            <TableCell className="text-right">{invoice.totalAmount}</TableCell>
          </TableRow>
        ))}
      </TableBody>
    </Table>

babblebey commented 1 year ago

I completely agree with this.

During the refactor, Its also imperative for us to be intentional in how we present for mobile, I found this article on how handle table for better mobile UX. Responsiveness for tables on mobile can be some strong pain in the neck.

https://link.medium.com/bvxaIM3d9Bb

brandonroberts commented 7 months ago

@nickytonline has some ideas around making the tables more responsive

open-sauced / app