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: miscellaneous fixes to table components #610

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from

Conversation

carissa-tang
Copy link
Collaborator

@carissa-tang carissa-tang commented Oct 30, 2023

Notion ticket link

N/A

Implementation description

  • Replace AdminTab with SearchableTablePage
  • Created EmptyUsersMessage for empty admin / teacher table
  • Replace deprecated isInline with HStack
  • Hide search filters if table has no initial results
  • Add search to SearchBar to clear input on tab switch

Steps to test

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@carissa-tang carissa-tang temporarily deployed to staging October 30, 2023 05:22 — with GitHub Actions Inactive
@github-actions
Copy link

Visit the preview URL for this PR (updated for commit a101fce):

https://jump-math-staging--pr610-carissa-fix-search-b-b1k0j8ac.web.app

(expires Mon, 06 Nov 2023 05:26:18 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c42d8d0d853b05885664a2dd73f8245f4333ae51

return (
<Center __css={styles}>
<MessageContainer
image={DisplayStudentsIllustration}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not the most fitting image, but i don't think we have another design for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair! Let's add a backlog ticket to ask design for a better image, but I'm fine with keeping this image for the mvp

Copy link
Contributor

@jfdoming jfdoming left a comment

Choose a reason for hiding this comment

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

Great! Just a couple of minor comments and one larger question about accessibility

@@ -44,7 +44,7 @@ const Pagination = ({
&lt; Previous
</PaginationPrevious>
)}
<PaginationPageGroup align="center" isInline>
<HStack align="center">
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to maintain semantic HTML I think we should use the library's built-in components (according to here the component already sets isInline so we sidestep the deprecation), or else duplicate all the other props that the library already sets.

{filterMenuComponent}
</HStack>
<VStack py={4} spacing={6} w="full">
{!noResults && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, have you checked that we never pass in filteredData.length for this prop anywhere? That would result in filters breaking the page, so you can probably just set the filters on every filter page to a combination with no results.

@@ -109,11 +104,13 @@ const DisplayAssessmentsPage = (): React.ReactElement => {
<SearchableTablePage
filterMenuComponent={<FilterMenu filterProps={filterOptions} />}
nameOfTableItems="assessments"
noResults={isEmpty}
noResults={(data?.tests?.length ?? 0) === 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also just do !data?.tests?.length

@@ -100,7 +100,7 @@ const DisplayClassroomAssessmentsPage = () => {
<QueryStateHandler error={error} loading={loading}>
<SearchableTablePage
nameOfTableItems="assessments"
noResults={paginatedData.length === 0}
noResults={(data?.class?.testSessions?.length ?? 0) === 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Suggested change
noResults={(data?.class?.testSessions?.length ?? 0) === 0}
noResults={!data?.class?.testSessions?.length}

<AdminTab
<SearchableTablePage
nameOfTableItems="teachers"
noResults={(teacherData?.teachers?.length ?? 0) === 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Suggested change
noResults={(teacherData?.teachers?.length ?? 0) === 0}
noResults={!teacherData?.teachers?.length}

return (
<Center __css={styles}>
<MessageContainer
image={DisplayStudentsIllustration}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair! Let's add a backlog ticket to ask design for a better image, but I'm fine with keeping this image for the mvp

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

Successfully merging this pull request may close these issues.

2 participants