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

Hide readviz by default #1556

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Hide readviz by default #1556

merged 2 commits into from
Jun 20, 2024

Conversation

rileyhgrant
Copy link
Contributor

Resolves #633

Hides readviz by default on Variant pages to try and save on egress fees.

Gives two options,

  • a button to load the data just this time
  • a checkbox to always load readviz data, this uses localStorage to save the value of this on load and when toggling this

Also adds tracking to these two buttons to try and see how often people use these

@rileyhgrant rileyhgrant force-pushed the hide-readviz-by-default branch from 97a53e7 to 9953610 Compare May 24, 2024 20:03
@rileyhgrant rileyhgrant self-assigned this May 24, 2024
@rileyhgrant rileyhgrant force-pushed the hide-readviz-by-default branch from 9953610 to 42a8a5a Compare May 30, 2024 21:25
@@ -307,8 +325,8 @@ class ReadData extends Component<ReadDataProps, ReadDataState> {
}

loadInitialTracks() {
;['exome', 'genome'].forEach((exomeOrGenome) => {
;['het', 'hom', 'hemi'].forEach((category) => {
;(['exome', 'genome'] as SequencingType[]).forEach((exomeOrGenome) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this works, it'd be more idiomatic to write ['exome', 'genome'].forEach((exomeOrGenome: SequencingType) => .... I think TS should be able to infer that properly. Same deal in the next line.

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 would have liked that better too, but unfortunately TS complains about doing it that way, as the forEach is being called on an array of strings.

<CheckboxInput
id="always-load"
checked={alwaysLoad}
onChange={(e: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

there should be a type you can use for e here, MouseEvent or something similar IIRC

@rileyhgrant
Copy link
Contributor Author

Screenshots of the UI:

  • Screenshot 2024-06-17 at 15 48 28
  • Screenshot 2024-06-17 at 15 48 17
  • Screenshot 2024-06-17 at 15 48 49

Copy link
Contributor

@phildarnowsky-broad phildarnowsky-broad left a comment

Choose a reason for hiding this comment

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

Cut, print, that's a wrap

Copy link
Contributor

@sjahl sjahl left a comment

Choose a reason for hiding this comment

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

This looks good to me from a UI / Text copy perspective

@rileyhgrant rileyhgrant force-pushed the hide-readviz-by-default branch 3 times, most recently from d43f30a to e8d03e6 Compare June 20, 2024 16:36
@rileyhgrant rileyhgrant force-pushed the hide-readviz-by-default branch from db93cca to a92857d Compare June 20, 2024 17:28
@rileyhgrant rileyhgrant merged commit e4e4178 into main Jun 20, 2024
4 checks passed
@rileyhgrant rileyhgrant deleted the hide-readviz-by-default branch June 20, 2024 17:44
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.

Hide readviz by default?
3 participants