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.

Sign up for GitHub

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

Jump to bottom

Fix #4292: Upgraded font-awesome and play/pause icons #4750

Merged
merged 2 commits into from
Mar 1, 2018
Merged

Fix #4292: Upgraded font-awesome and play/pause icons #4750

merged 2 commits into from
Mar 1, 2018

Conversation

bansalnitish
Copy link
Contributor

@bansalnitish bansalnitish commented Feb 27, 2018 •
edited
Loading

Explanation

Fixes #4292: Upgraded font awesome and play/pause icons.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
  • The PR explanation includes the words "Fixes #bugnum: ...".
  • The linter/Karma presubmit checks have passed.
    • These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
  • The PR is made from a branch that's not called "develop".
  • The PR follows the style guide.
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
    • If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

@bansalnitish
Copy link
Contributor Author

@tjiang11 can you also restart the tests. 9 out of 10 tests have passed and one has terminated due to time limit/ no. of jobs ?
I don't think this PR would lead to build failure.

tjiang11
tjiang11 approved these changes Mar 1, 2018
View reviewed changes
Copy link
Contributor

@tjiang11 tjiang11 left a comment

Choose a reason for hiding this comment

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

Hi @bansalnitish , this looks good, but after seeing it I think it just feels rather small. Do you think we can try increasing the size from 1.5em to 2em? Thanks!

@codecov-io
Copy link

codecov-io commented Mar 1, 2018 •
edited
Loading

Codecov Report

Merging #4750 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4750   +/-   ##
========================================
  Coverage    44.69%   44.69%           
========================================
  Files          385      385           
  Lines        23462    23462           
  Branches      3802     3802           
========================================
  Hits         10486    10486           
  Misses       12976    12976

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 b880727...541d029. Read the comment docs.

@bansalnitish
Copy link
Contributor Author

Hi @tjiang11, I have increased the size to 1.7em as 2em looked too large.
screenshot from 2018-03-01 12-24-26
Does this look fine?

@tjiang11
Copy link
Contributor

tjiang11 commented Mar 1, 2018

LGTM, thanks @bansalnitish !

@tjiang11 tjiang11 merged commit 38754ba into oppia:develop Mar 1, 2018
@bansalnitish bansalnitish deleted the icon-fix branch March 1, 2018 18:46
@seanlip seanlip mentioned this pull request Mar 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjiang11 tjiang11 tjiang11 approved these changes

Assignees

@tjiang11 tjiang11

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bansalnitish @codecov-io @tjiang11