feat: beacon validation and errors #125
Conversation
|
draft translation pr is here, just needs French portions filled in. |
davidlougheed
changed the title
| // complexity of instructions suggests the form isn't intuitive enough | ||
| const VARIANTS_INSTRUCTIONS_TITLE = 'Variant search'; | ||
| const VARIANTS_INSTRUCTIONS_LINE1 = | ||
| 'To search for all variants inside a range: fill both "Variant start" and "Variant end", all variants inside the range will be returned. You can optionally filter by reference or alternate bases.'; |
There was a problem hiding this comment.
i guess bento_public linting doesn't enforce this, but i'd prefer a max 120-char line width here
| // https://docs.genomebeacons.org/variant-queries/ | ||
|
|
||
| // as an alternative, we could require "end" always, then form logic would be less convoluted | ||
| // just set start=end to search for SNPs |
There was a problem hiding this comment.
when we eventually have chrom autofill from gohan/ref service, we could use the information in the ref service to auto-fill end to the end of the chromosome if it's unset (and change it if the user changes the chromosome to the length of the new chromosome if it === the previous chrom length) or something like this. for now, i think this is fine even though it's convoluted.
There was a problem hiding this comment.
Do people really want to search entire chromosomes?
There was a problem hiding this comment.
not really no, that's true
There was a problem hiding this comment.
could perhaps work for genes though I guess
| launchEmptyQuery(); | ||
| }; | ||
|
|
||
| const SearchToolTip = ({ text }: { text: string }) => { | ||
| const handleValuesChange = (_: Partial<FormValues>, allValues: FormValues) => { | ||
| form.validateFields(['Chromosome', 'Variant start', 'Variant end', 'Reference base(s)', 'Alternate base(s)']); |
There was a problem hiding this comment.
this list of fields is used once above too for const empty - maybe worth factoring out?
There was a problem hiding this comment.
yeah, there are a lot of variant form names lying around in the code (and not too helpfully the correct name for this collection is something like allVariantFormFieldsExceptAssemblyID... )
In empty we're checking each name individually, where in validate fields we're using all names together. So a possible refactor is something like:
const empty = !allVariantFormFieldsExceptAssemblyID.some(field => formValues[field])
but i'm not sure this is better....
There was a problem hiding this comment.
could use the every() function of the array (plus a boolean cast of !! for formValues[field])
in this case, why is assembly ID not validated?
There was a problem hiding this comment.
The reasoning behind the variants form is that if you want to search variants you fill it in, and if you don't, you leave it blank. So the if the form is blank it's ignored, and none of the rules about required fields are applied.
However, the form is almost never blank, since the assembly ID field is already filled for you in the (very common) case where there is only one assembly to search from. That's why it's treated separately from everything else.
| import React, { Dispatch, SetStateAction } from 'react'; | ||
| import { Alert } from 'antd'; | ||
|
|
||
| const BeaconErrorMessage = ({ message, setErrorAlertClosed }: BeaconErrorMessageProps) => { |
There was a problem hiding this comment.
I think a more generalizable parameter format would be to accept an onClose function, which is bound directly to the Alert - rather than passing a state setter down. this brings it more in line with antd components, and we could in the future change the onClose behaviour to do additional/different things.
There was a problem hiding this comment.
Ah, the state is here only to force a form re-render, it doesn't actually carry any useful information.
There was a problem hiding this comment.
yes the state is fine, I mean that the prop on BeaconErrorMessage should be onClose and that should be bound to the state update function where this component is used
| launchEmptyQuery(); | ||
| }; | ||
|
|
||
| const SearchToolTip = ({ text }: { text: string }) => { | ||
| const handleValuesChange = (_: Partial<FormValues>, allValues: FormValues) => { | ||
| form.validateFields(['Chromosome', 'Variant start', 'Variant end', 'Reference base(s)', 'Alternate base(s)']); |
There was a problem hiding this comment.
could use the every() function of the array (plus a boolean cast of !! for formValues[field])
in this case, why is assembly ID not validated?
Form error checking and user feedback for Beacon search UI:
Some new text requires translation.
The sheer amount of instructions and feedback suggests that the form could probably be more intuitive to use. There is some discussion here in the code.