Skip to content
This repository was archived by the owner on Dec 4, 2024. It is now read-only.

Draft: Add table component#46

Open
momotofu wants to merge 19 commits intoatomicjolt:mainfrom
momotofu:add-table-component
Open

Draft: Add table component#46
momotofu wants to merge 19 commits intoatomicjolt:mainfrom
momotofu:add-table-component

Conversation

@momotofu
Copy link
Copy Markdown
Contributor

No description provided.

@@ -0,0 +1,60 @@
import React, { useState } from 'react';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

here

@@ -0,0 +1,97 @@
import React from 'react';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And here

renderTableContent: (...arg: any[]) => any
}

export const composeTable = <T extends string | number>() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find the composeTable wrapper a bit clunky, but I suppose there's not much we can do about that to still enforce the generic Enum type for all T fields. I'd probably add this to the bottom of the file, so people don't have to use the composeTable for non-enum (and exclusively JS) tables

export const Table = composeTable<string>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If only you could generically type JSX!

desc = 'DESC',
}

export type ColumnType<T extends string | number> = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would probably add

type EnumValue = string | number;

And replace all the string | number with is. Not necessary, but it makes it more clear what string | number means in this context

searchColumn: T
headClasses?: string
onSearch: (arg: string) => void
renderTableContent: (...arg: any[]) => any
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not clear on how renderTableContent is supposed to operate. It says that it receives variables args here, but the call in the component doesn't pass any arguments

Copy link
Copy Markdown
Contributor

@seanrcollings seanrcollings Jun 29, 2021

Choose a reason for hiding this comment

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

searchTerm: 'Hello World',
searchColumn: FilterType.name,
onSearch: () => console.log('Sup'),
renderTableContent: (one = 'one', two = 2, three = 'III') => (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are these default argument values? The component doesn't pass anything, and these could just be locally scoped to this function

{ dataName: FilterType.dueAt, displayName: 'DUE' },
{ dataName: FilterType.createdAt, displayName: 'CREATED' },
{ dataName: FilterType.completed, displayName: 'COMPLETED' },
{ dataName: 'actions', displayName: 'actions', hidden: true },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if there would be a bit more elegant way to add extra columns without a title and such.
I couldn't think of one when I initially wrote this, but who knows?

sortDirection={sortDirection}
searchTerm={searchTerm}
onSearch={onSearch}
onSort={(newSortDirection: SortDirection, newSortColumn: T) => {
Copy link
Copy Markdown
Contributor

@seanrcollings seanrcollings Jun 29, 2021

Choose a reason for hiding this comment

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

onSort will need to be passed in as a property, or you'll need to pass sortDirection and sortColumn to renderTableContent

{ dataName: 'actions', displayName: 'actions', hidden: true },
]

Default.args = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It doesn't look like these are type checking properly. Values can be swapped for invalid ones and TS doesn't raise an error. Maybe look into that?
It appears that Default.bind() returns a type of any instead of an appropriate type and that's why it's not raising any complaints

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants