WIP Feature/layout - Githubissues

nbumbarger commented 7 years ago

Work in progress; requires modal styling. Based on https://github.com/openaq/openaq-upload/pull/7

danielfdsilva commented 7 years ago

I'm doing the review on this PR because is the one most advanced. Some of the comments are applied inline, but here are some others.

https://github.com/openaq/openaq-upload/blob/219cad42bd3811a17f3f233fd97b57f48dd0f4f4/app/assets/scripts/components/upload-form.js#L78 - Can it have any other value other than true|false? Should we default to something if not one of these?

https://github.com/openaq/openaq-upload/blob/219cad42bd3811a17f3f233fd97b57f48dd0f4f4/app/assets/scripts/components/upload-form.js#L91 - Maybe we should consider generalising this to handle other arrays, or is that something that is not going to happen?

nbumbarger commented 7 years ago

@danielfdsilva cheers. I had to do a refactor based on the design discussion we had this morning. This version should be very close to final; it still needs form validation and a success screen, which should be quick fixes in the morning. I also want to clean up the CSS variable names and fix these suggestions.

I am having trouble testing the upload from localhost due to a CORS issue. Is that something I can somehow work around on my end, or is it a server configuration? Could you take a look at this part of the upload-form component? cc @olafveerman

danielfdsilva commented 7 years ago

@nbumbarger What do you mean by testing locally? what's the url that is no working, the openAQ or amazon? If it's a CORS problem, should be handled on the server. I guess @jflasher will be able to provide a better answer.

Also, let me know when you address all the issues and I can do a final review.

nbumbarger commented 7 years ago

Regarding the two questions above:

According to the schema, mobile is a required boolean.
The attribution array idea was just one way I thought of for representing an array of pairs, where one is required and the other isn't. I'm not sure I understand what you mean by handling other arrays but I agree it's a pretty clumsy thing to do with CSV data, and I'm sure there are other ways. For example, name and url could be represented in the same cell, joined by special charecter if both existed.

I also fixed the inline comments, in cases where they still apply. There's a problem with the S3 URL that we haven't solved, so the upload can't actually succeed. It seems that the url is being currupted either in transit or at generation. There are messages for both failure and success, though, which you can by changing the 200 status code in the submit function. This is likely a problem that needs to be solved on the server, but it's also possible that we just need to send a different request. It would be great if you could help look into it, because it's the last real blocker.

jflasher commented 7 years ago

Can we do a quick check on Safari? It was complaining that fetch didn't exist when loading the page for me.

jflasher commented 7 years ago

And just a note that if we want to email people when their data gets ingested, we'll need to be capturing their email somewhere in the form.

danielfdsilva commented 7 years ago

@nbumbarger fetch is not yet available on safari and IE. you can use https://github.com/matthew-andrews/isomorphic-fetch to polyfill

nbumbarger commented 7 years ago

This branch has form validation bugs that need to be corrected, in addition to questions regarding e-mail submission https://github.com/openaq/openaq-upload/issues/12. The form validation bugs don't block file submission, so they'll be addressed in a separate bug/form-validation branch.

nbumbarger commented 7 years ago

Combined with https://github.com/openaq/openaq-upload/pull/19

openaq / openaq-upload

WIP Feature/layout #8