-
Notifications
You must be signed in to change notification settings - Fork 9
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: add deposition related filters #1079
Conversation
@kne42 I don't see the geneontology filter on the single dataset page on the dev link |
d337c26
to
5e35e13
Compare
filters={[GO_FILTER]} | ||
label={t('goId')} | ||
queryParam={QueryParams.GoId} | ||
prefix="GO:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we reference GO_PREFIX
?
const { t } = useI18n() | ||
const [searchParams, setSearchParams] = useSearchParams() | ||
|
||
const paramValue = searchParams.get(queryParam) ?? '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're just using paramValue
in a ternary, do we need the ?? ''
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done so it (and therefore value
) will be typed as a string always instead of string | null
const paramValue = searchParams.get(queryParam) ?? '' | ||
const [value, setValue] = useState(paramValue) | ||
|
||
const validatingRegex = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a comment explaining this regex?
@@ -71,6 +72,16 @@ export function getExpectedTotalCount({ | |||
) | |||
} | |||
|
|||
export function getPrefixedId({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - better to just have 2 arguments so the types are easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kne42 LGTM, thanks!
461118d
to
c5e8d4a
Compare
import { DropdownFilterButton } from './DropdownFilterButton' | ||
import { InputFilter } from './InputFilter' | ||
|
||
export function RegexFilter({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we should make this component more generic, cuz now we don't have a general component that's just an input field with an "Apply" right? Maybe regex
should be optional? (could just be a TODO too)
|
||
import { RegexFilter } from './RegexFilter' | ||
|
||
export function IdFilter({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this looks like it's supposed to be filtering CryoET entity prefixes specifically, maybe name it EntityIdFilter or something? 🤷
@@ -0,0 +1,3 @@ | |||
export enum IdPrefix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
label: string | ||
title: string | ||
queryParam: QueryParams | ||
prefix?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type this to IdPrefix
label={label} | ||
title={title} | ||
queryParam={queryParam} | ||
regex={validatingRegex} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the prop be called validationRegex
as well?
prefix?: string | ||
}) { | ||
const validatingRegex = useMemo( | ||
() => (prefix ? RegExp(`^(?:${prefix}-)?(\\d+)$`, 'i') : /^\d+$/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the filter isn't using the capture groups from the regex anymore, is it possible to just remove ?:
?
🤖 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>
#917
note: will not actually filter anything since backend is not hooked up yet
live link: https://dev-kira-1.cryoet.dev.si.czi.technology/