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

PATCH: changing to font icons as a temporary fix to style weather icons #69

Merged
merged 3 commits into from
Sep 14, 2015

Conversation

jodiedoubleday
Copy link
Contributor

What does this PR do? (please provide any background)

We were using SVG's as background images on our weather components, however this meant that the colors of the icons couldn't be changed.

  • We have moved to use Meteocons hosted on hungry geek (for the moment)
  • We will eventually use our own icons SVGs hosted on a CDN or in the HTML (for over-brandng)

What tests does this PR have?

Nothing new

How can this be tested?

  • run grunt build
  • run grunt docs
  • have a play with the weather component (you can edit int eh code box of the docs, it doesn't look like it but you can)
  • Change the font color of the icon in CSS

Screenshots / Screencast

screen shot 2015-09-11 at 13 15 36

#### What gif best describes how you feel about this work?

Face Palm


Developer Definition of Done/Quality Checklist (for PR author to complete BEFORE code review):

Software Engineer or Developer review:

  • I’ve witnessed the work behaving as expected (this could be on the authors machine or screencast).
  • I’ve checked for coding anti-patterns.
  • I’ve checked for appropriate test coverage.
  • I’ve checked all the tests are passing.

Software Engineer or project guru review:

  • I’ve witnessed the work behaving as expected (this could be on the authors machine or screencast).
  • I’ve checked for coding anti-patterns.
  • I’ve checked for appropriate test coverage.
  • I’ve checked all the tests are passing.

@hpoom
Copy link
Contributor

hpoom commented Sep 11, 2015

"We will eventually use our own icons fonts hosted on a CDN" - Would we not eventually move to SVG? I would assume we want to sunset all use of icon fonts in favour of SVG?

@jodiedoubleday
Copy link
Contributor Author

Yup wrong wording, changed in PR decription

@cameronviner
Copy link
Contributor

Taking dev

@cameronviner
Copy link
Contributor

Looks pretty solid to me 👍 https://cloudup.com/c6wAqyr8XRU

@jackdclark
Copy link
Contributor

Review 🙌

@@ -2,24 +2,35 @@
#Weather
\*------------------------------------*/

@font-face {
font-family: 'MeteoconsRegular';
Copy link
Contributor

Choose a reason for hiding this comment

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

linting comment: 4 spaces

@jackdclark
Copy link
Contributor

Couple of really minor linting things, code looks good though 👍.

Looking forward to getting it into HEHA!

rainbow

@jackdclark
Copy link
Contributor

👍

jodiedoubleday added a commit that referenced this pull request Sep 14, 2015
PATCH: changing to font icons as a temporary fix to style weather icons
@jodiedoubleday jodiedoubleday merged commit 941922b into master Sep 14, 2015
@jodiedoubleday jodiedoubleday deleted the changeIcons branch September 14, 2015 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants