-
Notifications
You must be signed in to change notification settings - Fork 257
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
feat: Can upload profile picture #20
Conversation
Awesome progress so far! Don't worry too much about a clean history in the PR; I will squash all the commits into one merge commit. |
After learning 3 or 4 ways one shouldn't try and interact with the ActiveStorage JavaScript, I finished the front-end implementation for things. Now I just need to test the validation logic out and write tests. |
ddd3c7e
to
5b20a95
Compare
I got this to a place that felt ready to merge. That said, it leaves behind a couple of TODO items:
With that said, I think the feature came out nicely. It should be tested in another local environment as well just to be careful. The features include rendering avatar images in all of the relevant UI areas, direct uploads of avatar images in the developer form, and validations on file info. Additionally, we might want to eventually look at generating variants to improve page load speeds. I looked into the docs for this and was a bit confused. I ultimately decided variants now were putting the cart before the horse, but just something to be aware of. |
Thanks for this PR @jacobdaddario! Awesome work getting the uploads together. I made a few changes to simplify the JavaScript and dependencies of the app.
I'd love to add back in the progress bar but overlaid on top of the image in the future! |
@joemasilotti Do you intend to add back in the file size verification at any point? It seems like it'd be really important. |
I do! Thanks for the reminder, I forgot to add it back in. |
Do you have any recommendations on learning more about how you wrote the custom validations? |
I always go back to the example in the Rails Guide. |
Just found this tutorial which explains how error messages are generated, too. |
FYI, just added #26. |
Taking a closer look at this now. I see you had to go to significant effort to revise the PR a bit. Do you have any suggestions that can help me cut down on the effort required by you in the future? I'm available to chat this evening if you'd like. |
That's on me, to be honest. I didn't document enough in the original issue to mention the vision I had for this feature. You did great work with the progress bar, and I'd love to incorporate that in the future, but I don't want that kind of JavaScript complexity in the app just yet. In the future, I'm going to do a better job of laying out more details of the features that I want completed. That said, I'm still happy to dive into a live code review if you'd like! |
Okay! It's always great to have code reviewed since I don't do this professionally. Let me know your availability and we can get something together that lines up with our work and life schedules. Maybe we can do something for #24. The drag and drop feature is proving to be interesting. Browsers don't let you programmatically alter file inputs for security reasons, so I had to do some exploring to get things functional. |
Just sent you a DM :) |
This is a WIP pull-request that resolves #11.
I've been having to get super into the guts of ActiveStorage in order to get some of the JavaScript solutions up-to date without incorporating super heavy-weight JS packages. I'm starting to get there, so I wanted to open a PR to mark my progress.
When I'm done, I'll do an interactive rebase and get the commits looking nicer. Plus, it'll give me a chance to throw out commits like
7048d0a
which are no longer necessary.