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: deposition filter banner #1040

Merged
merged 21 commits into from
Aug 29, 2024
Merged

Conversation

codemonkey800
Copy link
Contributor

@codemonkey800 codemonkey800 commented Aug 19, 2024

#925

  • deposition ID query parameter: deposition-id
  • prop banner?: ReactNode to render banner at the top of <TablePageLayout />
  • implements deposition banner component and renders at top of dataset or run pages

Demos

https://dev-depo-filter-banner.cryoet.dev.si.czi.technology/

Dataset Page

image

Run Page

image

@@ -61,6 +63,25 @@ export function RunsTable() {
[searchParams, setSingleDatasetHistory],
)

const getRunUrl = useCallback(
(id: number) => {
const url = createUrl(`/runs/${id}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - since we're only using the path and the query params, do we need createUrl()? what if carryOverFilterParams() just returned a new URLSearchParams and we did return `/runs/${id}\` + urlSearchParams.toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to prefer modifying the URL and URLSearchParams objects directly to avoid direct string concatenation, so createUrl() is helpful here since we're only using the pathname and params

@bchu1
Copy link
Contributor

bchu1 commented Aug 21, 2024

Another thought - now that we have the deposition ID in the URL params, should we just consolidate the previous deposition state into the query param? Unless there's a situation where we'd only want one of either the back to deposition button or the filter banner...

@codemonkey800
Copy link
Contributor Author

@Janeece it should be fixed now, could you take a look again when you get a chance please 🙏

Copy link
Collaborator

@Janeece Janeece left a comment

Choose a reason for hiding this comment

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

@codemonkey800 thank you, it looks great!

@codemonkey800 codemonkey800 merged commit bb83312 into main Aug 29, 2024
14 checks passed
@codemonkey800 codemonkey800 deleted the jeremy/deposition-filter-banner branch August 29, 2024 21:20
github-actions bot added a commit that referenced this pull request Sep 5, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.26.0](web-v1.25.0...web-v1.26.0)
(2024-09-05)


### ✨ Features

* add deposition related filters
([#1079](#1079))
([ffec095](ffec095))
* Add metadata sidebar for tomogram
([#1112](#1112))
([39351f0](39351f0))
* Add Tomograms Summary to run sidebar
([#1094](#1094))
([5a3132a](5a3132a))
* deposition filter banner
([#1040](#1040))
([bb83312](bb83312))
* info panel deposition metadata
([#1092](#1092))
([01a8b67](01a8b67))
* Make main photo HTML and CSS consistent between run and dataset pages
([#1096](#1096))
([6a88242](6a88242))
* More updates to tomograms table
([#1106](#1106))
([a6ef66e](a6ef66e))
* View Tomogram tooltips + button changes
([#1089](#1089))
([54b4fe5](54b4fe5))


### 🐞 Bug Fixes

* Carry over filters from datasets table
([#1113](#1113))
([b3c8628](b3c8628))
* Disable Apollo Client cache on the server
([#1088](#1088))
([373b987](373b987)),
closes
[#1041](#1041)


### ⚙ Continuous Integration

* add missing skip e2e
([#1116](#1116))
([ee6064a](ee6064a))
* skip e2e if feature disabled
([#1114](#1114))
([b5cb014](b5cb014))

---
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