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

feat: use react table to render resources subpage #313

Merged
merged 15 commits into from
Jul 19, 2024

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Jul 15, 2024

This update introduces a new 'Datatable' component implementation that replaces the prior 'DashboardTable.'

Also update implementation to fit design.

The 'Datatable' leverages the '@tanstack/react-table' library to enhance data table capabilities. It includes features like pagination, loading states, empty placeholders, and customizable cells and headers. Additionally, the PR updates various related file components and ensures that the project dependencies are aligned with these changes.

Copy link

vercel bot commented Jul 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
isomer-studio ❌ Failed (Inspect) Jul 19, 2024 7:30am

Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

what do we feel about using a bottom up approach to building the types rather than using a top-down approach (everything is using ResourceTableData) here - i think this is cos we haven't locked in the types yet so i think it's ok for now

i think the Datatable component here might still be doing abit too much here, it feels like we're trying to export a big component that can do everything and i think allowing a bit more flexibility might make more sense here cos it feels like a thin wrapper anw

@@ -39,7 +39,7 @@ export function CmsSidebarContainer({
{sidebar}
</Box>
</GridItem>
<GridItem as="main" area="main">
<GridItem as="main" area="main" overflow="hidden">
Copy link
Contributor

Choose a reason for hiding this comment

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

@sehyunidaaa - seekign input on this!

Comment on lines +60 to +86
{isFetching && (
<>
<Flex
// white alpha to denote loading
bg="whiteAlpha.800"
bottom={0}
left={0}
p="1rem"
pos="absolute"
right={0}
top={0}
zIndex="1"
/>
<Flex
bottom={0}
flex={1}
left={0}
pos="fixed"
right={0}
top={0}
w="100vw"
zIndex={2}
>
<Box m="auto">
<Spinner />
</Box>
</Flex>
</>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

this component has to care about loading states but actually what i feel is the right abstraction here would be for the component here to only care about rendering the table and not care about loading.

Suspense feels like a good fit here, we can omit hte isLoading and it'll still fit nicely! wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cannot use suspense. We do not want the full fallback. We want to show the previous items in the table still, which suspense will not do (since it will swap out the current rendered table items with the suspense item completely).
That is also the reason why data fetching is still done with useQuery and not useSuspenseQuery.

The loading state is a semi-transparent overlay.

apps/studio/src/components/Datatable/Datatable.tsx Outdated Show resolved Hide resolved
apps/studio/src/components/Datatable/Datatable.tsx Outdated Show resolved Hide resolved
Comment on lines +15 to +21
if (date === "folder") {
return (
<Text as="span" textStyle="caption-2">
-
</Text>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatives we can consider:

  1. showing last edited time of direct child pages
  2. showing last edited time of all child pages + folder

i think here just using - is actually good enough but just cc @sehyunidaaa if you care. i think using - is actually really good for me and i hate 2 + dislike 1.

Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

lgtm! some points which i didn't agree with initially but landed on a good middle ground fokr the future:

  1. presenting a powerful API for data-table makes sense here because our use-case here is very narrow. we don't foresee this component changing anytime soon so ok to just hide stuff inside here.
  2. Suspense won't quite work here because the loading state which we want to show is dependent on the current data, which Suspense doesn't handle well (it wants a fallback component).

again, nbd on both points - lgtm!

Copy link
Contributor Author

karrui commented Jul 19, 2024

Merge activity

@karrui karrui force-pushed the add_sidebar_for_sidelist_page branch from 1ba97f9 to 8c21103 Compare July 19, 2024 07:14
Base automatically changed from add_sidebar_for_sidelist_page to main July 19, 2024 07:19
@karrui karrui force-pushed the use_react_table_to_render_resource_table branch from ef041c2 to fea224b Compare July 19, 2024 07:20
@karrui karrui force-pushed the use_react_table_to_render_resource_table branch from fea224b to 56b7c08 Compare July 19, 2024 07:29
@karrui karrui merged commit f5fc950 into main Jul 19, 2024
20 of 23 checks passed
@karrui karrui deleted the use_react_table_to_render_resource_table branch July 19, 2024 07:37
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.

2 participants