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

Alpha Tag #758

Merged
merged 1 commit into from
Apr 4, 2018
Merged

Alpha Tag #758

merged 1 commit into from
Apr 4, 2018

Conversation

willsputra
Copy link
Contributor

Description

Added alpha tag

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

Fixes: #685

@@ -1126,20 +1126,17 @@ input.is-invalid {
}

#alpha{
color: #a78600;
color: #000000;
Copy link

@mkosowsk mkosowsk Apr 1, 2018

Choose a reason for hiding this comment

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

Bookkeeping but think we are just using `color: black;' throughout the codebase. @thelostone-mc?

Copy link
Member

Choose a reason for hiding this comment

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

Well I believe we had agreed to stick to the hex color, so as to keep it uniform throughout ! Else we end up with names in a few areas and hex code in the rest !

Copy link

Choose a reason for hiding this comment

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

Whoops! @willsputra please disregard :)

font-weight: 100;
padding: 2px 7px;
font-weight: 100;
font-family: 'Muli', sans-serif;
Copy link

Choose a reason for hiding this comment

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

Indentation off on this line and previous

@mkosowsk
Copy link

mkosowsk commented Apr 1, 2018

@willsputra dropped some super minor comments for ya to take a look at when you get a moment :) Overall LGTM but I did not test. @thelostone-mc got some cycles? 👍🏻

@willsputra #685 (comment) are the images per this comment up to date or has this changed a bit? If there are some changes please add updated pics to #685 when you get a moment but if not disregard. Thanks!

Great work! :)

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.

@mkosowsk Yup pulled it in and checked it out ^_^
Everything looks solid except for the tag on the leaderboard page on mobile + indentation fix

@willsputra Could you check that out ? Let's get this merged post that

screen shot 2018-04-01 at 8 24 05 pm

@mbeacom mbeacom added needs-testing frontend This needs frontend expertise. labels Apr 2, 2018
@mbeacom
Copy link
Contributor

mbeacom commented Apr 2, 2018

Per @willsputra Screen

@codecov
Copy link

codecov bot commented Apr 2, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #758   +/-   ##
=======================================
  Coverage   33.95%   33.95%           
=======================================
  Files         101      101           
  Lines        5695     5695           
  Branches      666      666           
=======================================
  Hits         1934     1934           
  Misses       3685     3685           
  Partials       76       76

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 8eba434...159bd79. Read the comment docs.

@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 2, 2018

@willsputra rebase gone wrong ? :P There are 15 commits
Easiest Solution : cherry pick the commits. Lemme me know if you need help with that :)

@willsputra
Copy link
Contributor Author

@thelostone-mc yeah might need your help sorry 😞 would have to improve my git knowledge!

@thelostone-mc
Copy link
Member

Pinging you over slack !!

@thelostone-mc
Copy link
Member

@mbeacom @mkosowsk Everything taken care off in one nice commit 😎

x

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

@mkosowsk
Copy link

mkosowsk commented Apr 2, 2018

LGTM, this PR has earned the Mitch Kosowski Seal of Approval™ 💪🏻🤠👍🏻

@willsputra mind sharing with the community what technique you used to get down to 1 commit? 👍🏻

@thelostone-mc truly the community is in the presence of a Git master 😳

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 @PixelantDesign Are you good with the attached visual changes?

@willsputra
Copy link
Contributor Author

with the guidance of Git master @thelostone-mc :

  1. made a new branch
  2. cherrypicked the chosen commits with git cherry-pick
  3. squashed two commits to one with git rebase -i HEAD~2
  4. reset and push the original branch with
    git checkout 685
    git reset --hard foobar
    git push origin -f 685

@PixelantDesign PixelantDesign changed the title Fixes: https://github.com/gitcoinco/web/issues/685 Alpha Tag Apr 3, 2018
@mbeacom mbeacom merged commit 364f352 into gitcoinco:master Apr 4, 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.

as a user, i want to ahve an 'in alpha' badge on alpha tools, so i can know which tools aren't yet GA
5 participants