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

Clothing #3843

Merged
merged 7 commits into from
Feb 27, 2019
Merged

Clothing #3843

merged 7 commits into from
Feb 27, 2019

Conversation

mehul0698
Copy link
Contributor

@mehul0698 mehul0698 commented Feb 25, 2019

Description
Checklist
Affected core subsystem(s)
Refers/Fixes
Testing and Sign-off
Contributor
  • Read and followed the Contributor Guidelines
  • Tested all changes locally
  • Verified existing functionality
  • Ran make test and everything passed!
Reviewer
  • Affirm contributor guidelines have been followed and requested changes made
  • CI tests and linting pass
  • No conflicts (migrations, files, etc)
  • Regression tested against staging or local deployment
Funder
  • Validated requested changes were made to specification
  • Bounty payout released to the contributor

@mehul0698
Copy link
Contributor Author

image
image
image
image
image
image
image
image
image
image

@mehul0698 mehul0698 mentioned this pull request Feb 25, 2019
5 tasks
@codecov
Copy link

codecov bot commented Feb 25, 2019

Codecov Report

Merging #3843 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3843      +/-   ##
==========================================
- Coverage   29.51%   29.47%   -0.04%     
==========================================
  Files         201      201              
  Lines       15590    15590              
  Branches     2047     2047              
==========================================
- Hits         4601     4595       -6     
- Misses      10836    10842       +6     
  Partials      153      153
Impacted Files Coverage Δ
app/avatar/utils.py 14.8% <ø> (ø) ⬆️
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f82c73...50e3d9e. Read the comment docs.

@mehul0698
Copy link
Contributor Author

@owocki @thelostone-mc please review these designs once

@thelostone-mc
Copy link
Member

I LOVE :D
Oh could we extend the sleeves on the round neck and collar shirt ? It looks like they have no arms 😂

@mehul0698
Copy link
Contributor Author

mehul0698 commented Feb 26, 2019

@thelostone-mc extending the sleeves will spoil the design and once the avatar is saved its not visible

@mehul0698
Copy link
Contributor Author

@thelostone-mc will this not work without extending the sleeves?

@thelostone-mc
Copy link
Member

It would work but the issue the image is a square!
So when folks download it -> they avatar would not have arms

@mehul0698
Copy link
Contributor Author

image

@mehul0698
Copy link
Contributor Author

image
image
image

@mehul0698
Copy link
Contributor Author

@thelostone-mc I have updated the 2 designs. Please review it once.

@mehul0698
Copy link
Contributor Author

@owocki please review this once.

@owocki
Copy link
Contributor

owocki commented Feb 26, 2019

screenshots LGTM ... defer to @thelostone-mc from here

@mehul0698
Copy link
Contributor Author

@owocki from the doctor design should I put the stethoscope as a separate accessory?

@owocki
Copy link
Contributor

owocki commented Feb 26, 2019

i think its fine the way it is

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

LGTM ^_^

@thelostone-mc thelostone-mc merged commit ecfe4c5 into gitcoinco:master Feb 27, 2019
@mehul0698
Copy link
Contributor Author

mehul0698 commented Feb 27, 2019

@owocki the pr has been merged. I consider 3 of these to be exotic. You owe me 140 dollars.

@gitcoinbot
Copy link
Member

⚡️ A tip worth 1.00000 ETH (138.33 USD @ $138.33/ETH) has been granted to @mehul0698 for this issue from @owocki. ⚡️

Nice work @mehul0698! Your tip has automatically been deposited in the ETH address we have on file.

@gitcoinbot
Copy link
Member

Nyan Cat ⚡️ A *Nyan Cat* Kudos has been sent to @mehul0698 for this issue from @owocki. ⚡️

Nice work @mehul0698!
Your Kudos has automatically been sent in the ETH address we have on file.

@mehul0698
Copy link
Contributor Author

@owocki will you accept anymore clothing designs?

@owocki
Copy link
Contributor

owocki commented Feb 27, 2019

@mehul0698 let me know what you had in mind and we can take it case by case from there!

@mehul0698
Copy link
Contributor Author

mehul0698 commented Mar 5, 2019

@owocki @thelostone-mc I have made a space suit, a pilots uniform, a baseball jersey, an american football jersey and an armour. If you like any of the ideas just let me know I'll put a pr.

@owocki
Copy link
Contributor

owocki commented Mar 5, 2019

i like all of them!

@owocki
Copy link
Contributor

owocki commented Mar 5, 2019

can the american football jersey come with a helmet ? :)

@mehul0698
Copy link
Contributor Author

@owocki Yes the helmet is possible. You want it as a separate accessory or along with the clothing?

@owocki
Copy link
Contributor

owocki commented Mar 5, 2019

along w. clothing sounds good to me

@mehul0698
Copy link
Contributor Author

@owocki the helmet wont be in front as the avatars are composed of layers, the clothing layer is the bottom most layer so the head is always going to be above the clothing layer so the helmet will become hidden. Under the current restriction the helmet is only possible as a separate accessory as the accessories are the topmost layer.

@mehul0698 mehul0698 deleted the clothing branch March 5, 2019 19:57
@owocki
Copy link
Contributor

owocki commented Mar 5, 2019

oh sorry when you said "along with the clothing" i thought we were talking about in the same PR.

totally makes sense to have it as an accessory layer item to me!

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

Successfully merging this pull request may close these issues.

5 participants