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: Refactor <TablePageLayout> to support multiple tabs #911

Merged
merged 16 commits into from
Jul 25, 2024
Merged

Conversation

bchu1
Copy link
Contributor

@bchu1 bchu1 commented Jul 23, 2024

#624

This is to support having both Annotations and Tomograms tabs on the Runs page.

  • Adds support for a header above the tabs.
  • Uses our existing <Tabs> component.
    • Only shows tabs if more than 1 is specified.
  • Supports a different pagination query param per tab.
    • This changes the query param on the Runs page from page to annotations-page. In a subsequent PR I'll add tomograms-page.
    • It seems like we're currently pretty far from either annotations or tomograms getting to a point where they'd actually trigger pagination, but I'd prefer this to work properly from the get-go rather than find out there's a bug later.
  • Stores the currently open tab in URL params.
  • Make a few prop names clearer.
  • Allow <Tabs> to take in components as labels and numbers (i.e. indexes) as values.
  • Also enable hoisting in ESLint (which TS checks anyways).

@bchu1 bchu1 changed the title Refactor <TablePageLayout> to support multiple tabs feat: Refactor <TablePageLayout> to support multiple tabs Jul 23, 2024
@bchu1 bchu1 changed the title feat: Refactor <TablePageLayout> to support multiple tabs chore: Refactor <TablePageLayout> to support multiple tabs Jul 23, 2024
@bchu1 bchu1 changed the title chore: Refactor <TablePageLayout> to support multiple tabs feat: Refactor <TablePageLayout> to support multiple tabs Jul 23, 2024
@ehoops-cz
Copy link
Contributor

Do we think people may have the page query param bookmarked? If so, do we want a redirect to annotations-page? This may be overkill, but I think it's worth considering.

return prev
},
{ replace: true },
)
}
}, [filteredCount, page, setSearchParams])
}, [filteredCount, page, pageQueryParam, setSearchParams])
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing that page and pageQueryParam are two different things. Can we rename one?
tableTypeQueryParam? dataTypeQueryParam? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed both.

@bchu1
Copy link
Contributor Author

bchu1 commented Jul 25, 2024

Do we think people may have the page query param bookmarked? If so, do we want a redirect to annotations-page? This may be overkill, but I think it's worth considering.

Definitely a good thing to keep in mind. Right now nothing on the runs page ever exceeds 1 page so we should be good.

Comment on lines 55 to 62
title={i18n.filterNoResultsFound}
description={i18n.filterTooRestrictive}
actions={<Button onClick={reset}>{i18n.clearFilters}</Button>}
/>
),
filteredCount: filteredDatasetCount,
totalCount: datasetCount,
countLabel: i18n.datasets,
Copy link
Member

Choose a reason for hiding this comment

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

while you're here, would you mind updating to using useI18n()/t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bchu1 bchu1 merged commit 659bea3 into main Jul 25, 2024
5 checks passed
@bchu1 bchu1 deleted the bchu/table branch July 25, 2024 20:27
github-actions bot added a commit that referenced this pull request Jul 26, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.14.0](web-v1.13.1...web-v1.14.0)
(2024-07-25)


### ✨ Features

* Refactor &lt;TablePageLayout&gt; to support multiple tabs
([#911](#911))
([659bea3](659bea3))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants