-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Refactor ProfileImage #5803
Refactor ProfileImage #5803
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.
I ❤️ this so much, I think it's way easier for the reader of the code. Thanks @seb1441!
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!
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.
Great refactor, thanks for the PR!
Shoot, we waited a little too long on this! @seb1441 any chance you can resolve the conflict that we have? Or maybe @rhymes can do it quickly since he is very familiar with those API builders. |
For some reason, probably permissions, I can't resolve this conflict: I rebased the branch manually to see where the conflict was and the issue is that These are the steps to do it manually: $ git rebase master # with master up to date
$ git rm app/views/api/v0/users/me.json.jbuilder
$ git rebase --continue
$ git push -f # if you have permission to write to this PR |
a43f004
1dedbd1
to
a43f004
Compare
@seb1441 it looks like there are a few broken specs this time around https://travis-ci.com/thepracticaldev/dev.to/builds/147858632 - likely new specs using the old signature, that entered with the rebase :) Sorry for this! |
a43f004
to
083ae33
Compare
@rhymes Newly added tests fixed :) |
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.
Thanks for fixing this @seb1441!
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.
Thank you @seb1441 for your patience with this, I plan on getting this merged later today(unless someone beats me to it) to avoid any more conflicts!!!
What type of PR is this? (check all applicable)
Description
As suggested by @citizen428, here's the minor refactor to go from
ProfileImage.new.get(150)
toProfileImage.new.get(width: 150)
. This makes theget
method more explicit on what it actually does and also allows later, if needed, to easily add more optional arguments likeheight
.Related Tickets & Documents
#5583 (comment)
Added to documentation?