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

Predictor upload components #631

Merged
merged 49 commits into from Oct 2, 2019
Merged

Conversation

rwblair
Copy link
Collaborator

@rwblair rwblair commented Aug 1, 2019

Base components for selecting a dataset, selecting a variety of files to upload, adding descriptions for predictor names from uploaded file present. Things on the todo list:

  • Post forms, run ids, and predictor descriptions to api.
  • Add some way to observe upload progress
  • Component to display users existing runs
  • Function to handle clicking on tab titles in predictor upload form.
  • Have cancel function clear form state for predictor upload form.
  • Add link to /mycollections route in user menu
  • Render tests for added components

…pi.ts

Move dataset columns to helper compoents to allow it to be shared
between analysis builder and predcitor collections uploader. Added
initial components for selecting dataset, selecting files and then
selecting runs to be assocaited with files. Does not post to api though
on submit.
…s from

upload form. Make setting content type header optional on a jwt fetch.
…Using these create new form to take descriptions for the headers
@codecov-io
Copy link

codecov-io commented Aug 1, 2019

Codecov Report

Merging #631 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #631      +/-   ##
=========================================
- Coverage   81.82%   81.8%   -0.03%     
=========================================
  Files          57      57              
  Lines        2223    2231       +8     
=========================================
+ Hits         1819    1825       +6     
- Misses        404     406       +2
Impacted Files Coverage Δ
neuroscout/schemas/predictor.py 100% <100%> (ø) ⬆️
neuroscout/tasks/compile.py 87.09% <0%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e78e4d...971056d. Read the comment docs.

@adelavega
Copy link
Collaborator

I got this error compiling:


/neuroscout/neuroscout/frontend/src/predictor_collection/AddPredictorsForm.tsx
(2,18): Unused declarations are forbidden

@adelavega
Copy link
Collaborator

Well only with yarn start. Compiled with yarn build

@adelavega
Copy link
Collaborator

Weird, I'm also getting a CORS error when I switch the host to alpha. Will try locally.

@adelavega
Copy link
Collaborator

Overall it's looking pretty good! Here's some minor feedback

Screenshot from 2019-08-22 18-38-02

  • The buttons and lines from the box are quite close. A tiny bit of space would make things look cleaner.
  • I think it would be better if the file picker for the first file was open by default, rather than having to click "Add" even for the first file.
    We'll also need more instruction here, maybe on the first page, as its a bit bewildering without. Maybe at the top of each "Tab" we can have a bit of explanatory text.
  • "Add more" rename to -> "Add event file".
  • I think it would be better if it said "OK" instead of "Hide". Also, it would be better if that hid the whole section including the file picker, and instead just showed the name of the file and the Edit button (Let's call it just "Edit")
  • Although there are "Prev" and "Next" buttons, clicking on the tab doesn't navigate. That's probably OK but it has a hand icon when you mouse over, so its a bit confusing.
  • Also, about the buttons, I think having the "Cancel" button the Left makes more sense. Or potentially we could get rid of it, since there's an "X" at the top. And instead have the "Back"/"Next" button where it is. This would be a bit more like the Analysis builder.
  • No colon ":" after "Collection Name" to match the style of the rest of the app. Same goes for the whole app in general.

Aside from those tweaks, it looks pretty good to me!

In terms of future changes, we might want to let users click on existing collections and get more info about the variable names etc... but we can get to that later (when we discuss permissions, and whether these are live or not). I'll show this to Tal tomorrow if I get a chance.

@adelavega
Copy link
Collaborator

adelavega commented Aug 23, 2019

  • Private/public flag on Predictor - default public (in model) but set to private when creating PredictorCollection.
  • Only show private predictors if they match user ID (modify GET predictors API route). Also add user's private predictors to what is returned.
  • Add private/public status display to "My Collections"
  • Add documentation (and how to contact us to make predictors public)

…ser informaiton. analysis builder and predictor collection updated to respect this. Updated predictor api schema to return dataset_id to help determine when predictors from predictor collections should be included in available predictors in analysis builder
…hey are not present in the available predictors list
…ade testing libraries where applicable, update old tests to pass.
@adelavega
Copy link
Collaborator

Having some compile problems:

Failed to compile.

/neuroscout/neuroscout/frontend/src/predictor_collection/__tests__/PredictorDescriptionForm.test.tsx
(8,25): Property 'testPredictor' does not exist on type 'typeof import("/neuroscout/neuroscout/frontend/src/predictor_collection/__tests__/data")'.


error Command failed with exit code 1.

@rwblair
Copy link
Collaborator Author

rwblair commented Oct 2, 2019

latest 3 commits fixes the inactive datasets in predictor collection form and the google signle sign on avatar not showing on refresh.

@rwblair rwblair changed the title [WIP] Predictor upload components Predictor upload components Oct 2, 2019
@rwblair
Copy link
Collaborator Author

rwblair commented Oct 2, 2019

@adelavega what text did you want for the "no data" in the predictor colleciton list?

I put in a yarn build in place of frontend tests until I get ssh enabled on travis to sort out the yarn tests.

@adelavega
Copy link
Collaborator

"No custom predictors uploaded"

That should be good enough for now. Since so few people will look at this, we can add more info when we work on the docs.

@adelavega adelavega merged commit 632270d into neuroscout:master Oct 2, 2019
0.9 automation moved this from In Progress to Done Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.9
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants