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

file upload functionality and error hijacking #111

Merged
merged 48 commits into from
Apr 18, 2019
Merged

file upload functionality and error hijacking #111

merged 48 commits into from
Apr 18, 2019

Conversation

mikeal
Copy link
Member

@mikeal mikeal commented Jan 3, 2019

Still a work in progress, but the basics are there.

@mikeal
Copy link
Member Author

mikeal commented Jan 3, 2019

I implemented this as a mixin but I'm not so sure that's the best approach. Most of the code still has to live in the main template and code eval anyway, so we could just add the functionality to Lesson and just have the lesson author add the "isFileLesson" property.

@mikeal
Copy link
Member Author

mikeal commented Jan 3, 2019

Also, I'm not sure how we should handle directories (if at all). There's a way to do it but I think it's Chrome only https://github.com/feross/drag-drop/blob/master/index.js#L121

@lidel
Copy link
Collaborator

lidel commented Jan 25, 2019

Oli did a deep dive on this recently to fix drag&drop upload error and I think we have code that works across Firefox/Chrome in ipfs-webui.

@olizilla are you able to link to relevant snippets or share some thoughts on best practices?

- Fix trailing comma linter error
- Address merge conflicts while getting this branch up to date
with current site features.
@terichadbourne
Copy link
Member

terichadbourne commented Jan 29, 2019

I do think we'll want to demonstrate some things using directories, so tips from @olizilla on the implementation @lidel mentioned would be great. (See #91 (comment) for the kinds of commands I'd like users to be able to explore from within the tutorial.)

@lidel
Copy link
Collaborator

lidel commented Feb 6, 2019

In case it helps, I tracked some prior art:

Digression: directories complicate things, initial lesson may want to deal with uploading files alone. Later, we could show how to create directories on the fly (#91 (comment)), and finally we could tackle directory uploads.

Remove unused properties in lesson with no exercise
- Add dummy file tutorial lesson
- TODO: Figure out why exercise box isn't appearing
- Offer multiple boilerplates
- IN PROGRESS: Update instructions for building a lesson
- Update UI for file upload when `ifFileLesson` is true, adding upload
box and different instructions for submission
- Update lesson content to explain new exercise format
- Fix error preventing exercise box from appearing in file lessons
- Move "Step 2" header into `v-if` so that it only shows in lessons where
file uploaded is needed
- Remove unused console logs
- Add file tutorial to homepage
- Adjust dummy titles and descriptions
@terichadbourne
Copy link
Member

terichadbourne commented Feb 13, 2019

@mikeal I've pushed the changes we worked on today to

  • Troubleshoot why upload click is still making the file selector open twice in a row. (Drag and drop functions as intended.)
  • Make the validation happen when the submit button is clicked, NOT when the file is uploaded. (Flow should be that the user uploads files, updates code editor, then hits submit to test.)
  • Turn uploadedFiles from a binary into an array of file names uploaded and print them to page in place of the placeholder content.

- Appease linter
- Make a consistent look and feel for the upload box and the resulting
box that shows what files you've uploaded
- TODO: Figure out how to make new file icon be aqua like the add one
- Make formatting consistent between reset text links
- Add checkmark icons when you upload your code and when you change the
code in the editor
@terichadbourne
Copy link
Member

Made additional UI improvements based on some of the feedback from today's design team and GUI team calls.

Suggestions to consider that aren't yet implemented:

  • Make the new file icon aqua like the add icon (I don't know much about SVGs and this one is put there in a different way from the other so I need to study up on how to do that, but it seems like an easy one to knock off)
  • Replace "Reset Code" and "Start Over" text links with some sort of consistent reset icon
  • Disable submit button until both steps one and two are completed OR gray out and disable the code editor until the file upload is complete. (I don't love the latter idea since it doesn't really matter in what order you do the steps.)
  • Separate instructions for step 1 and instructions for step 2 (this would require a pretty decent refactor of how our markdown files and Vue files interact

- Add border to file upload box when file is dragged over it
- Print filenames from actual array of uploaded files
- Fix issue where file explorer was opening twice on click
- Add args to run function
- Edit code in File-Basics/01.vue to be what would be the successful code
for an add lesson (TODO: remove part of this code and update validation
elsewhere)
- Change document icon to aqua SVG
- Fix formatting of <ul> element for better design when multiple files are
uploaded
- Disable submit button in file lesson when !uploadedFiles
- Add a note indicating this and make it even more prominent during
hover over disabled button (have to hack it with a span b/c disabled
buttons don't have a hover property)
- Disallow folder upload since we can't access the details we need in
the browser. Users can successfully upload either a single file or
multiple files through either drag-and-drop or selection.

Pair programmed with @mikeal
- Add a 2nd lesson to file tutorial series
- Update dummy content in both lessons
- TODO: Fix error messages in lesson 1
- TODO: Create real content for lesson 2
- Change opacity rather than font weight when user hovers over disabled
submit button's parent element. (Warning that you need to upload a file
now does not appear until that hover thanks to 0 opacity.)
- Minor text edits based on feedback from design team.
- Complete lesson 1 exercise
- TODO: Fix error which prevents fail/success messages from showing if
 you try to submit the default code as your code
- Add exercise to practice adding a file to IPFS.
- TODO: Fix validation
- More attempts to get the right info to use for validation
- TODO: Actually validate
@fsdiogo
Copy link
Collaborator

fsdiogo commented Apr 3, 2019

@fsdiogo Does share.ipfs.io use only MFS or was a workaround found to use the other methods as well?

share.ipfs.io does not uses MFS at all, it's just plain simple adding and getting!

@terichadbourne
Copy link
Member

@olizilla tells me that the way the GUI team has been making Web UI work with browser file objects is by wrapping them in readable streams, and of course I'd rather avoid having to teach ProtoSchool users how to do that. @hugomrdias has it in his OKRs for the upcoming quarter to make the non-MFS add() work with blobs. 🎉

@olizilla is on the same page as @mikeal about teaching MFS first, and given that I can push ahead on that with blob support now, my plan is to focus this PR on an MFS tutorial and later circle back to see what we can effectively teach on the non-MFS commands in the browser.

@alanshaw I know you're depending on some of this content for your IFPS Camp tutorial, but I'm hoping this prioritization will be okay. 🤞

- Rename tutorial according to new focus on the Mutable File System
- Update routes and index listings accordingly
- Add lesson on `files.ls`
- Update validation error message
@terichadbourne
Copy link
Member

I've restructured this tutorial to be just about MFS, have removed the old ipfs.add lesson, and have added an ipfs.ls() lesson.

I'd love feedback from @fsdiogo @mikeal or others on whether the content of the existing lessons is actually true and whether I can simplify any of the existing validation. Good tips now will help me be more efficient moving forward. Thanks!

@terichadbourne
Copy link
Member

@fsdiogo Heads up that I've just added content for the next lesson (files.mkdir) but haven't yet fixed up the validation.

@terichadbourne terichadbourne changed the title [WIP] File upload lesson [WIP] MFS tutorial + file upload functionality Apr 11, 2019
@fsdiogo
Copy link
Collaborator

fsdiogo commented Apr 16, 2019

@terichadbourne @olizilla can you please take a look at the error overriding and the instructions for using it, more or less in 6a2f2ee and 68e8e36?

@terichadbourne as this branch is getting a bunch of work done in it, we should fork it to different branches so our work won't collapse, but specially to ease the code reviews.

I've commented the imports to the MFS lesson in my last commit, so we can merge this branch to code. You should then fork code and continue your work on the MFS lessons; I'll fork code too, to add the console outputs in another branch; @olizilla will make his own branch too from code and get on with his work as well.

Sounds good?

@fsdiogo fsdiogo mentioned this pull request Apr 17, 2019
@fsdiogo
Copy link
Collaborator

fsdiogo commented Apr 17, 2019

Ping @terichadbourne @olizilla ☝️ (my previous comment)

@terichadbourne
Copy link
Member

@fsdiogo I’m all for your plan for getting this merged and splitting out work off into new branches. I should be able to make some time to proof this today, but I’d prefer if someone more familiar with IPFS than I am also proofed the new validation system for hijacking error messages. @mikeal or @olizilla could you please help to review?

@terichadbourne terichadbourne changed the title [WIP] MFS tutorial + file upload functionality [WIP] file upload functionality and error hijacking Apr 17, 2019
@terichadbourne
Copy link
Member

@fsdiogo I've clarified and added to your instructions for using the new error override feature.

The end result is working is for me as a user when I uncomment the routing to get the MFS lessons usable - I see the custom errors when needed but can make native syntax errors and such pass through unimpeded.

@terichadbourne terichadbourne changed the title [WIP] file upload functionality and error hijacking file upload functionality and error hijacking Apr 17, 2019
@terichadbourne
Copy link
Member

@fsdiogo, @mikeal just gave the official “LGTM” after looking at your commits for the error hijacking. Feel free to merge if you’re comfortable with my latest commit.

@fsdiogo fsdiogo merged commit c3de28b into code Apr 18, 2019
@fsdiogo fsdiogo deleted the file-upload branch April 18, 2019 09:22
@@ -7,6 +7,7 @@ cache:
before_script:
npm install
script:
npm test
Copy link
Collaborator

@olizilla olizilla Apr 18, 2019

Choose a reason for hiding this comment

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

@fsdiogo needs more hyphens:

script:
  - npm test
  - npm run build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants