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

Changes Material Icons with Font Awesome icons in these files #16797

Closed
wants to merge 7 commits into from

Conversation

852hamza
Copy link

@852hamza 852hamza commented Dec 29, 2022

Overview

  1. This PR fixes or fixes part of #[15968].
  2. This PR does the following: [Replace Material Icons with Font Awesome icons]

Essential Checklist

  • The PR title starts with "Fix Replace Material Icons with Font Awesome icons #15968: ", followed by a short, clear summary of the changes. ( oppia/core/templates/components/question-directives/questions-list/questions-list.component.html, oppia/core/templates/pages/learner-group-pages/create-group/add-syllabus-items.component.html, oppia/core/templates/pages/skill-editor-page/editor-tab/skill-prerequisite-skills-editor/skill-prerequisite-skills-editor.component.html".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

Proof that changes are correct

  1. oppia/core/templates/components/question-directives/questions-list/questions-list.component.html
    Before changes

Screenshot from 2022-12-17 19-31-36

After changes

Screenshot from 2022-12-17 19-36-12

2.oppia/core/templates/pages/learner-group-pages/create-group/add-syllabus-items.component.html

Before changes
Screenshot from 2022-12-28 00-19-51
Screenshot from 2022-12-28 00-20-13
Screenshot from 2022-12-28 00-20-26
Screenshot from 2022-12-28 00-37-09

After changes

Screenshot from 2022-12-28 00-41-50
Screenshot from 2022-12-28 00-42-01
Screenshot from 2022-12-28 00-42-56
Screenshot from 2022-12-28 00-44-14

  1. oppia/core/templates/pages/skill-editor-page/editor-tab/skill-prerequisite-skills-editor/skill-prerequisite-skills-editor.component.html

Before changes
Screenshot from 2022-12-27 23-28-56

After changes
Screenshot from 2022-12-27 23-37-18

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
  • Never force push. If you do, your PR will be closed.
  • Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

@852hamza 852hamza requested a review from a team as a code owner December 29, 2022 02:46
@oppiabot
Copy link

oppiabot bot commented Dec 29, 2022

Hi @852hamza, can you complete the following:

  1. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
    Thanks!

@oppiabot
Copy link

oppiabot bot commented Dec 29, 2022

Hi @nithinrdy, could you please add the appropriate changelog label to this pull request? Thanks!

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Dec 31, 2022
@oppiabot
Copy link

oppiabot bot commented Dec 31, 2022

Hi @852hamza, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

Copy link
Member

@nithinrdy nithinrdy left a comment

Choose a reason for hiding this comment

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

Looks good overall, just one issue.

For each of the new check marks that you've added, could you please position them a little bit lower so that they properly align with the option text present next to them? Adding a bit of margin/padding should do the trick.

PTAL, Thanks!

@852hamza
Copy link
Author

852hamza commented Jan 2, 2023

Okay

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 3, 2023
Copy link
Author

@852hamza 852hamza left a comment

Choose a reason for hiding this comment

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

kindly review

@nithinrdy
Copy link
Member

Could you please add updated images after the changes? With the check mark now better aligned with the text?

@852hamza
Copy link
Author

852hamza commented Jan 3, 2023

Could you please add updated images after the changes? With the check mark now better aligned with the text?

Screenshot from 2023-01-03 11-26-36

Screenshot from 2023-01-03 10-58-08

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 5, 2023
@oppiabot
Copy link

oppiabot bot commented Jan 5, 2023

Hi @852hamza, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 9, 2023
@852hamza
Copy link
Author

852hamza commented Jan 9, 2023

@nithinrdy

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 11, 2023
@oppiabot
Copy link

oppiabot bot commented Jan 11, 2023

Hi @852hamza, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 17, 2023
@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 19, 2023
@oppiabot
Copy link

oppiabot bot commented Jan 19, 2023

Hi @852hamza, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 25, 2023
@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 29, 2023
@oppiabot
Copy link

oppiabot bot commented Jan 29, 2023

Hi @852hamza, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot
Copy link

oppiabot bot commented Jan 31, 2023

Hi @852hamza, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks!

@oppiabot
Copy link

oppiabot bot commented Feb 7, 2023

Hi @852hamza, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale label Feb 7, 2023
@oppiabot oppiabot bot closed this Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Material Icons with Font Awesome icons
2 participants