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/cleanup old GWAS UI app code #1231

Merged
merged 4 commits into from
Feb 10, 2023

Conversation

pieterlukasse
Copy link
Contributor

@pieterlukasse pieterlukasse commented Feb 9, 2023

Jira Ticket: VADC-399

Improvements

  • cleanup of OLD GWAS UI app code, keeping only V2 of the app and the Results viewer app

Deployment changes

@pieterlukasse pieterlukasse changed the base branch from master to feat/vadc_sprint03 February 9, 2023 16:21
Copy link
Contributor

@jarvisraymond-uchicago jarvisraymond-uchicago left a comment

Choose a reason for hiding this comment

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

Great work Pieter! I pulled the changes locally and confirmed only one GWAS app remains without causing any obvious errors.

Before merging consider

  • Do we want to rename the folder for the application from GWASV2 to GWAS now that there's only one GWAS left?
  • For tickets like this: https://ctds-planx.atlassian.net/browse/VADC-323 should the developer reference this PR to find all the removed storybooks?
  • Are there follow up tickets to implement the needed changes in other deployment related config files?

@@ -225,21 +225,6 @@
"react/no-unused-prop-types": "off"
}
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment, no changes needed:

This was probably a bad approach to begin with. Better would have been to ignore the line or the entire file rather than adding a new global rule for one file.

@pieterlukasse
Copy link
Contributor Author

Do we want to rename the folder for the application from GWASV2 to GWAS now that there's only one GWAS left?

Good point, although it is probably best to think of a more unique name, maybe GWASApp. Also, with "GWASV2" string having 162 hits in 37 files, this might be a bit large of a change to include here on top of all the other changes. I would prefer to do this in a separate action. There are some more cleanup actions that need to take place, e.g. the src/Analysis/GWASResults/GWASUIApp.css is probably way too large (a copy of the legacy versions) and is also referenced in a number of places where it really should not be.

Are there follow up tickets to implement the needed changes in other deployment related config files?

I've update the description with links to the other related PRs

@pieterlukasse pieterlukasse merged commit 2064fc3 into feat/vadc_sprint03 Feb 10, 2023
@pieterlukasse pieterlukasse deleted the feat/cleanup_old_gwasui_code branch February 10, 2023 15:55
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.

2 participants