Imageomics / dashboard-prototype

Prototype data dashboard for Imageomics Data
http://dash.imageomics.org
MIT License
5 stars 2 forks source link

Feature/upload #25

Closed egrace479 closed 1 year ago

egrace479 commented 1 year ago

Added upload option to Dashboard (csv or xls).

egrace479 commented 1 year ago

This is currently what it looks like before upload, then below the line gets replaced by the dashboard as it has been. Clicking on "Upload Data" again allows for a new upload.

Main Page

Should I add the column requirements underneath as well? It currently will throw an error (missing column, wrong file type, etc.) and point the user to how-it-works: Error Message (missing column)

hlapp commented 1 year ago

This is currently what it looks like before upload, then below the line gets replaced by the dashboard as it has been. [...] Should I add the column requirements underneath as well? It currently will throw an error (missing column, wrong file type, etc.) and point the user to how-it-works

It seems helpful to provide a link to "How it works" before someone runs into an error unintentionally because they didn't fully understand which columns they'd have to have.

egrace479 commented 1 year ago

It seems this change set lumps together changes for multiple separate and not necessarily interrelated issues. Generally I'd suggest breaking changes up into meaningful and coherent pieces, this makes review and feedback much easier.

Overall this looks fine but I notice there's a fair amount of code duplication. In the conditional for whether geo-coordinates are available or not, it seems only a small part differs for the two cases, and the rest is duplicated. There are also strings or string prefixes (such as https://github.com/Imageomics/dashboard-prototype/raw/main/test_data/images/) that occur many times over, and could just be defined at the beginning and then reused.

For this PR, would it have been better to do the file retrieval generalization with a revised preloaded dataset and test as its own PR, then move the main div out separately and add the upload feature?

To your second point, I can adjust the geo-coordinate if statement to append the final div to hist_div dependent on the boolean. For the string duplication, are you referring to test_query? I can adjust that as well.

egrace479 commented 1 year ago

Updated as per @hlapp's suggestions.

New landing page: landing page Updated Error message page with embedded link that opens in new tab too: error page

egrace479 commented 1 year ago

Is there a CSV I can test these changes with?

In test_data:

johnbradley commented 1 year ago

Thanks @egrace479. I was able to upload the the first two CSV files listed in the previous comment. Everything seemed to work fine for me.