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

Adding download page and hooking it into annotation tool workflow #91

Merged
merged 4 commits into from
Mar 28, 2022

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented Mar 28, 2022

This PR adds a basic download page that follows the annotation page.

Currently it is always accessible from the annotation page (not unlocked upon completion of annotation - like the other pages). This is just for demo purposes. Going to the download page while in the annotation page without having 'annotated' any data merely produces a blank page with "download" button.

However, once an annotation has occurred, going to the download page produces a table of the "annotated" data.

NOTE: Some other aspects of this implementation should be revisited after the demo tomorrow - including how/when annotation status is saved in the store. This will include moving replicated functionality like the 'Confirm and Upload' button upwards out of the annotation page's components.

@jarmoza jarmoza requested a review from surchs March 28, 2022 15:13
@surchs
Copy link
Contributor

surchs commented Mar 28, 2022

@jarmoza: Thanks for this PR! I only have one comment: why do we need to force-navigate to the download page every time we update the dataTable? With the demo in mind, that'd make you jump back and forth a lot (e.g. look at age -> "upload/confirm" -> be brought to download page -> navigate back to annotation -> ...).

I think it should be enough to just enable the download page once we have made one dataTable update emit and then let the user navigate to the download page on their own.

@jarmoza
Copy link
Contributor Author

jarmoza commented Mar 28, 2022

why do we need to force-navigate to the download page every time we update the dataTable

Well, we don't need to do that. It's a UI choice. Ideally, we'd want some sort of feedback from the upload button. The most immediate choice was to have the user move along in the annotation process (e.g. to the download page). Currently, there is no feedback as to what's happened otherwise.

Another possibility is to mimic the UI of the previous pages and to enable a 'Next' button once the user clicks the 'Confirm and Upload' button.

@surchs
Copy link
Contributor

surchs commented Mar 28, 2022

Yeah, I think I'd prefer if we just make the download page accessible conditional on dataTable.annotated not being null / the annotation page having received at least one dataTable update event

@jarmoza
Copy link
Contributor Author

jarmoza commented Mar 28, 2022

Okay. I'll add the button then.

@jarmoza
Copy link
Contributor Author

jarmoza commented Mar 28, 2022

@surchs Okay. Take a look and see what you think.

@jarmoza
Copy link
Contributor Author

jarmoza commented Mar 28, 2022

Additional note: I had tried to put the top part of the annotation page in a row - thus the indented top part, but row styling made it shrink in width. So the download page button is just in its own row beneath the annotation UI.

Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

@jarmoza: works well, I like it.

One comment, one suggestion

  • suggestion: let's call the button "Download" (I suggested inline)
  • comment: let's remve the pageData props (see comment)

With these two, looks good to go!

:disabled="!pageData.download.accessible"
:to="'/' + pageData.download.location"
:variant="nextPageButtonColor">
Next step: Categorize columns
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Next step: Categorize columns
Next step: Review and download the harmonized data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggestion: let's call the button "Download" (I suggested inline)

Oops! Left over from my copy-paste from the landing page. Will replace.

comment: let's remve the pageData props (see comment)

Good catch! Use of pageData here is leftover from a previous implementation where it was necessary as a prop for components since the 'Confirm and Upload' button(s) reside inside them – and was being used to proceed to the download page. Passing it through the annotation components is no longer necessary.

@@ -194,7 +194,11 @@ export default {
},
columns: {
type: Object
}
},
pageData: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are handling the button from the annotation page, can we remove the pageData props? It seems that they are not used inside the components they are being passed to. And we have the navigation component to handle the shared nav-state in one place for us.

@@ -12,6 +12,7 @@
:active-category="activeCategoryName"
:columns="columns"
:data-table="dataTable"
:pageData="pageData"
Copy link
Contributor

Choose a reason for hiding this comment

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

see other pageData comment

@surchs
Copy link
Contributor

surchs commented Mar 28, 2022

thus the indented top part

OK, let's squash the PR then so we don't get confused by all the styling commits

…f pageData as a prop for annotation page components
@jarmoza jarmoza merged commit 7fd633d into master Mar 28, 2022
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

great. looks good to me! 🎉

@surchs surchs deleted the download-page branch March 28, 2022 18:45
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