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

Fixes: https://github.com/gitcoinco/web/issues/792 #827

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

willsputra
Copy link
Contributor

Description

Separated profile and username to 2 lines.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
Testing
Refers/Fixes

Fixes: #792

@mbeacom mbeacom added frontend This needs frontend expertise. needs design labels Apr 7, 2018
@mbeacom mbeacom requested a review from PixelantDesign April 7, 2018 02:54
@mbeacom mbeacom added the profile label Apr 7, 2018
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
Copy link
Member

thelostone-mc commented Apr 7, 2018

@willsputra Conflicts :P

Oh could you throw in a screenshot, so that's it easier for folks to review ^_^

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.

@willsputra Could we

  • re-indent the css files to 2 spaces as against 4
  • run npm run stylelint:fix and fix the css errors

And throw in a screenshot so that we make @PixelantDesign 's life easier for reviewing and get this merged :)

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

You'll need to run make fix, make fix-stylelint, or npm run stylelint:fix to resolve the Travis failures. Can you also include the requested screenshots or a gif of the proposed changes per @thelostone-mc ?

@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 9, 2018

@mbeacom you just copied what I said :P
I got to this before you \m/
Aditya 1 - Mark 100 something

@mbeacom
Copy link
Contributor

mbeacom commented Apr 9, 2018

@thelostone-mc 🤷‍♂️ Just going down the list! 😅

@willsputra
Copy link
Contributor Author

oops thanks @mbeacom @thelostone-mc lemme try it out in a bit!

@codecov
Copy link

codecov bot commented Apr 9, 2018

Codecov Report

Merging #827 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #827   +/-   ##
=======================================
  Coverage   33.97%   33.97%           
=======================================
  Files         101      101           
  Lines        5774     5774           
  Branches      672      672           
=======================================
  Hits         1962     1962           
  Misses       3733     3733           
  Partials       79       79

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 a5bc27d...b9aff40. Read the comment docs.

@thelostone-mc
Copy link
Member

@willsputra squash + screenshot !!

@willsputra
Copy link
Contributor Author

screenshot here:

screenshot 2018-04-09 22 58 04

@mkosowsk
Copy link

mkosowsk commented Apr 9, 2018

@willsputra looking good! I haven't looked at the latest changes so you may have already done this but make sure to do npm run stylelint:fix and fix the css errors... @thelostone-mc and @mbeacom please make sure to let people know to do this, it's very important! 👍

@mbeacom
Copy link
Contributor

mbeacom commented Apr 9, 2018

@mkosowsk Per the contributor guidelines, you should install pre-commit - This will alert you before committing that your branch won't pass Travis CI.

@thelostone-mc
Copy link
Member

@PixelantDesign This looks alright ? ^_^

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm :shipit:

@mbeacom mbeacom merged commit 49d7aa0 into gitcoinco:master Apr 9, 2018
@willsputra
Copy link
Contributor Author

apologies for the hiccup guys 🙏

@mbeacom
Copy link
Contributor

mbeacom commented Apr 9, 2018

@willsputra All good! Thank you for your contribution!

@willsputra willsputra deleted the 792 branch April 9, 2018 15:35
@mkosowsk
Copy link

mkosowsk commented Apr 9, 2018

@willsputra no worries and congrats on the contribution!

I was just trying to be a smartass and give @mbeacom a hard time for repeating what @thelostone-mc said 😂 but then he went and provided some good insight 🤔

@mbeacom mbeacom mentioned this pull request Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend This needs frontend expertise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alpha tag confusion
5 participants