-
Notifications
You must be signed in to change notification settings - Fork 27
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/web 224/profile pic uploader #332
Feat/web 224/profile pic uploader #332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are the biggest things that stand out to me. I'm going to hold off on fine-tooth combing it until after you've linted and cleaned up your code.
Take your time, there's no rush on this 😄
I've fixed the comments and delinted. Only one thing I'm not 100% happy about, which is a render glitch on profile page load. Very briefly shows you the upload button. See what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall pretty good stuff. For the future, I would recommend looking into integrating ESLint and StyleLint into your IDE/Editor. You'll get real-time feedback and often times automatic fixes / suggestions. There's extensions for each on most modern code editors.
I'm still looking into that CSS issue with the modal opener button. I'll let you know when I find out why. If it's simple enough I'll just apply the fix along with the merge to develop.
@Pero-Moretti Just those couple things there about your image processing code and I think we're all good. |
Just leaving this note here. I'm going to merge this PR once the final change is in. Any further changes/fixes (mainly that graphical bug) should be made in a follow-up |
… works, but not in the way the end user will expect.
All done. |
Yeah probably best to forbid it just for the sake of leaving them with the expectation that animated images are not supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Now de-linted and resolved initial PR comments.