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

Updating minaa webapp #1

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

Sophiayangg
Copy link

No description provided.

@Sophiayangg Sophiayangg reopened this Dec 19, 2024
@crsl4
Copy link
Member

crsl4 commented Dec 22, 2024

@reednel If you have time to review, please do! It is not urgent, so don't prioritize this right now. I will take a look early January.

@Sophiayangg
Copy link
Author

Will test it on the server and push a new version after the testing.

@reednel
Copy link
Member

reednel commented Jan 9, 2025

Hey Sophia, great work and sorry for the delay. I have just a few comments for you.

  1. Please update the Copyright year at README.md:19, LICENCE:3, and aboutTab.R:122.
  2. In the DESCRIPTION file, set the author to yourself or Claudia. I assume it's just a web metadata thing but still this webapp is not mine to claim authorship of. And I'd recommend setting the version to 1.0.0 now, and applying a release tag of v1.0.0 to the suitable commit in Github later.
  3. If I'm correct in the observation that merge.R and visualize.R are not being used at all, they can be deleted. Should the need arise later, there's always the git history.
  4. Please update the README.md with something like a Development Instructions section describing how to get started with a local instance of the app.
    • Need R Studio...R version x.y.z...run [this] code [here] to install the requisite packages...etc.
    • Related side note: tonight I tried to run a local build of the app, and got the error there is no package called ‘shinyWidgets’. I haven't touched R in a couple years so after install.packages("Shinywidget") failed citing an unsupported version of R, I just gave up on an inspection of the UI.
  5. The following is just a code aesthetic thing, but in each of aboutTab.R, alignTab.R, resultTab.R, and visualizeTab.R, you define some CSS classes at the top. I believe it would be better practice to define all of those in a single style file and import that to each of these tabs, but I'm not sure if Shiny permits such a thing. In any case, this only impacts 4 files, you'll get my approval whether or not you address that.

@reednel reednel requested review from crsl4 and reednel January 9, 2025 19:35
@crsl4
Copy link
Member

crsl4 commented Jan 24, 2025

@reednel Can you review this at your convenience?

@crsl4
Copy link
Member

crsl4 commented Jan 24, 2025

@Sophiayangg For #4, you can use this readme as a template: https://github.com/solislemuslab/bioklustering

Copy link
Member

@reednel reednel left a comment

Choose a reason for hiding this comment

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

I'd like to see the README made more developer-friendly but otherwise I think it's great. I look forward to seeing this app up on wid.wisc.edu

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.

3 participants