Optional Three Letter Abbreviation for amino acid right table #330
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
If the goal is to modify how amino acids letters are rendered, is it not logical to change the actual rendering code, rather then patching the source data somewhere half-way?
Here is where the letters are rendered:
covariants/web/src/components/Common/MutationBadge.tsx
Lines 168 to 170 in a3ef0c7
| 'Ala': '#EAEABA', | ||
| 'Val': '#EAEA9F', | ||
| 'Leu': '#E1E177', | ||
| 'Ile': '#C9C94D', | ||
| 'Cys': '#E3F9B0', | ||
| 'Asp': '#E98F6D', | ||
| 'Glu': '#F7B080', | ||
| 'Phe': '#C7C88D', | ||
| 'Gly': '#C0C0C0', | ||
| 'His': '#D6F6FA', | ||
| 'Lys': '#CEC0F3', | ||
| 'Met': '#C3ED3C', | ||
| 'Asn': '#F29290', | ||
| 'Pro': '#D2D1F8', | ||
| 'Gln': '#F8C4E3', | ||
| 'Arg': '#A6ACEF', | ||
| 'Ser': '#D8B9D4', | ||
| 'Thr': '#F0D6E3', | ||
| 'Trp': '#86B0CC', | ||
| 'Tyr': '#8FC7D1', |
There was a problem hiding this comment.
Any reason for duplicating colors? Duplication is typically bad and increases maintenance and probability of mistakes.
There was a problem hiding this comment.
I think we have two options for the definition of the left/right colors this is one and the other is to translate from TLA to OLA in the color function.
However, I am looking at your feedback and it may not be necessary to change the color component
| function parseToTLA(ola: string): string { | ||
| return get(AMINOACID_TLA, ola, ola) | ||
| } | ||
|
|
||
| function parseToOLA(tla: string): string { | ||
| return get(AMINOACID_ONE, tla, tla) | ||
| } |
There was a problem hiding this comment.
This does not parse anything. Why is it called "parse"?
| @@ -55,18 +55,41 @@ export function hasDefiningMutations(cluster: ClusterDatum) { | |||
| } | |||
|
|
|||
| export function DefiningMutations({ cluster }: DefiningMutationsProps) { | |||
| const [isTLA, setTLA] = useState(true) // State changer for TLA or OLA | |||
There was a problem hiding this comment.
Any reason you decided to go with local state?
It is being reset on component unmount (e.g. page navigation), and now you need to pass the flag everywhere. Which prevents you from implementing the toggle for other places, which are far away from the button.
| <Button style={{ | ||
| padding: 0, | ||
| width: 125, | ||
| background: "#ffffff", | ||
| borderRadius: 10 , | ||
| color: '#495057' , | ||
| font: "Open Sans" , | ||
| fontSize: 14 , | ||
| textAlign: 'center' | ||
| }} | ||
| onClick = {() => setTLA(isTLA => !isTLA)}> | ||
| {label} | ||
| </Button> |
There was a problem hiding this comment.
Why you decided to go with a button? I thought a checkbox or a flip toggle is a more natural choice for a boolean.
|
I was hoping that we could implement a global toggle for all badges. Was this not the intent? Perhaps the side bar on these particular pages is not the best place for a toggle, because when switching to other pages user loses access to the toggle. |
|
Hi @carlos-montano-hub - thank you for this PR! I think it really shows that the transition between the 1-letter and 3-letter abbreviations can be smooth, and that the side-bar with the mutations on the Variants pages looks good in both formats. I tried the current preview at a variety of resolutions/mobile devices and I think it looks good. However I do think it's worth addressing some of Ivan's concerns above. In particular, it would be great for this to be a switch that adjusts how the badges display so that all badges across all parts of the page (in the text, in the non-defining mutation counts) and even across pages (Shared Mutations page) can change when the user requests a change, so that it's a 'universal state' across the site. Do you think that's possible? I also agree that a slider or radio button might work better as it more clearly indicates the current selection, and might also be able to be made a little more compact, to fit at the top(-ish) of the relevant pages (each Variant page & the Shared Mutations page, I think) where it would be used. However I think this shows really well, as I said, that the 3-letter abbreviations look good and the transitioning can be smooth, which makes it an idea worth pursuing and a great start! I'm hopeful to see it become something we can get integrated! |
|
Hello, thanks for the feedback @ivan-aksamentov @emmahodcroft I will be working on it and let you know |
Added a button to change the right table from One Letter Abbreviation to Three Letter Abbreviation in order to solve #323
In case of this PR being accepted, please accept #325 first, as I copied the code from there and in that way @chatelao would not lose his contribution
Any feedback please let me know