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

Remove all-caps default in /tip (yge) #195

Merged
merged 2 commits into from
Jan 2, 2018

Conversation

bumi
Copy link
Contributor

@bumi bumi commented Dec 30, 2017

Description

Similar to #116 and discussed in #151 this PR removes the global (on body) text-transform: uppercase style from the /tip (yge) application.

As discussed in #151 properties like the bounty status should also be normalized (capitalized in the UI)
These properties are stored in the DB and we need to decide where to put that presentation logic.

Checklist
  • normalize bounty properties (status, etc.)
Affected core subsystem(s)

ui

Refers/Fixes

@codecov
Copy link

codecov bot commented Dec 30, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #195   +/-   ##
=======================================
  Coverage   12.21%   12.21%           
=======================================
  Files          66       66           
  Lines        3120     3120           
  Branches      343      343           
=======================================
  Hits          381      381           
  Misses       2739     2739

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 417b692...5026a84. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Dec 31, 2017

is it just me, or would it be good if headers were still uppercase?

screenshots follow

screencapture-localhost-8080-tip-send-2-1514753821341
screencapture-localhost-8080-tip-send-2-1514753839333
screencapture-localhost-8080-tip-1514753877701
screencapture-localhost-8080-tip-1514753882162

@bumi
Copy link
Contributor Author

bumi commented Jan 2, 2018

we somehow should unify the different stylesheets that are used.
The /explorer has a special h1-h6 style for that (https://github.com/gitcoinco/web/blob/master/app/assets/v2/css/gitcoin.css#L31) but that stylesheet is not used in /tip.
Unifying the base stylesheets would make it easier.
Sadly I am not a frontend developer, any ideas on how to best approach that?

By loading the gicoin.css stylesheet the headlines in yge will be
uppercase as in the rest of the application.
This is a first step to try to move towards a unified stylesheet
for the two different components of the app.
@owocki owocki merged commit 797568b into gitcoinco:master Jan 2, 2018
@owocki
Copy link
Contributor

owocki commented Jan 2, 2018

awesome, thank you :)

@gitcoinbot
Copy link
Member

⚡️ A tip worth 0.02 ETH ($17.27) has been granted to @bumi for this issue from Kevin. ⚡️

The sender had the following public comments:

LOWERCASE FTW :)

Nice work @bumi, check your email for further instructions.

@@ -21,7 +21,7 @@
</span>
<h1>Send Tip ⚡</h1>
<h3>It's Fast. It's Easy. It's Free.️</h3>
<h4>(Supports any github username or email address)</h4>
<p>(Supports any github username or email address)</p
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nooo... I commited broken HTML.
As this PR is already merged, this is fixed in: a6a4c7d#diff-88a1050efecc8245bddec63cb35b9412R24

BUT as that other PR is kinda big, shall I do a quick PR to fix only this error, that we easily can merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

npnp ill fix it directly on master

@gitcoinbot
Copy link
Member

This issue now has a funding of 0.05 ETH (42.76 USD) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $12276.26 more Funded OSS Work Available at: https://gitcoin.co/explorer

@gitcoinbot
Copy link
Member

The funding of 0.05 ETH (42.84 USD) attached has been claimed by @owocki.

@owocki, please leave a comment to let the funder (@owocki) and the other parties involved your implementation plan. If you don't leave a comment, the funder may expire your claim at their discretion.

@owocki
Copy link
Contributor

owocki commented Jan 3, 2018

accidental bounty. claiming it myself now

@gitcoinbot
Copy link
Member

The funding of 0.05 ETH (42.84 USD) attached to this issue has been approved & issued to @owocki.

@bumi bumi deleted the design/151-lowercase-typo branch January 3, 2018 08:48
@bumi
Copy link
Contributor Author

bumi commented Jan 3, 2018

any idea how we should approach the bounty status text normalization?
It seems there is some stuff in python and some stuff in some JS.

@owocki
Copy link
Contributor

owocki commented Jan 3, 2018

sorry, im not sure i follow the question.

@bumi
Copy link
Contributor Author

bumi commented Jan 3, 2018

I was referring to the second topic in #151

ethikz pushed a commit to ethikz/web that referenced this pull request Jan 24, 2018
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.

3 participants