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

migrate react-fontawesome to offical package- BB-358 #317

Merged
merged 2 commits into from Dec 2, 2019
Merged

migrate react-fontawesome to offical package- BB-358 #317

merged 2 commits into from Dec 2, 2019

Conversation

sbvkrishna
Copy link
Contributor

@sbvkrishna sbvkrishna commented Nov 12, 2019 •
edited

Problem

react-fontawesome doesn't display some icons (brands): BB-358(closed)⏩ BB-363

Solution

Migrating react-fontawesome to official react-fontawesome
Related ticket: https://tickets.metabrainz.org/browse/BB-363

Areas of Impact

Client- Almost all pages which use FontAwesome icons, including Index page, Layout etc

@coveralls
Copy link

coveralls commented Nov 12, 2019 •
edited

Coverage Status

Coverage increased (+0.07%) to 43.088% when pulling 9777d01 on sbvkrishna:react-fontawesome into f7e54d0 on bookbrainz:master.

@sbvkrishna
Copy link
Contributor Author

sbvkrishna commented Nov 12, 2019 •
edited

Completed

  • Setup the Migration

Migrated and Tested-

  • IndexPage- src/client/components/pages/index.js
  • CallToAction- src/client/helpers/entity.js
  • Layout- src/client/containers/layout.js

Pending

subtask

    • src/client/components/forms/deletion.js
    • src/client/components/loading-spinner.js
    • src/client/components/pages/entities/author.js
    • src/client/components/pages/entities/edition-table.js
    • src/client/components/pages/entities/edition.js
    • src/client/components/pages/entities/footer.js
    • src/client/components/pages/entities/image.js
    • src/client/components/pages/entities/work-table.js
    • src/client/components/pages/parts/editor-profile.js
    • src/client/components/pages/parts/search-field.js
    • src/client/entity-editor/alias-editor/alias-editor.js
    • src/client/entity-editor/button-bar/alias-button.js
    • src/client/entity-editor/button-bar/identifier-button.js
    • src/client/entity-editor/common/linked-entity.js
    • src/client/entity-editor/common/new-date-field.js
    • src/client/entity-editor/common/validation-label.js
    • src/client/entity-editor/edition-section/edition-section.js
    • src/client/entity-editor/identifier-editor/identifier-editor.js
    • src/client/input.js

Cleanup (Post-migration)

MonkeyDo
MonkeyDo reviewed Nov 12, 2019
View reviewed changes
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR , it looks good, working nicely and also adds a couple of bonus improvements!

I think the pending changes will make good entry level GCI tasks, thanks for making a clear list.
Each task involves using git to clone the project, getting the project running, creating an account on the issue tracker and assigning a ticket and making a PR, it's a nice scope for starting contributors.

The way I see it work, each pending change needs to be turned into sub-tasks of a JIRA ticket we can point participants to, that will contain more detailed instructions (I can help write this one).
Each task should have a similar short description with the path of the file to change and pointing to the main task for more info.
Are you up for doing that?

package.json
@@ -39,6 +39,10 @@
},
"dependencies": {
"@fortawesome/fontawesome-free": "^5.4.0",
Copy link
Contributor

@MonkeyDo MonkeyDo Nov 12, 2019 •
edited

Choose a reason for hiding this comment

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

This package is not going to be used after everything is changed over.
We'll need to create a sub-task to remove the package and anywhere fontawesome-free appears in the codebase as well as the static assets

src/client/helpers/entity.js Show resolved Hide resolved
@sbvkrishna
Copy link
Contributor Author

Thank you for the review. And yes I'm up for this!

@sbvkrishna sbvkrishna marked this pull request as ready for review November 12, 2019 13:32
MonkeyDo
MonkeyDo requested changes Nov 12, 2019
View reviewed changes
src/client/stylesheets/style.less Outdated Show resolved Hide resolved
MonkeyDo
MonkeyDo approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

OK, it all works quite well for me now.

Good work, and thanks for investigating all the details !

I'll leave this PR open for now, and look at merging it right before GCI starts to reduce the in-between period (using both the old and new packages).

@MonkeyDo
Copy link
Contributor

Related ticket: https://tickets.metabrainz.org/browse/BB-363

@sbvkrishna
migrate(font-awesome): install official fontawesome packages and setu…
358539f 
…p icon library

Add "@fortawesome/fontawesome-svg-core","@fortawesome/free-brands-svg-icons","@fortawesome/free-solid-svg-icons","@fortawesome/react-fontawesome"
packages. Also, setup icons library.
@sbvkrishna
migrate(font-awesome): migrate pages- index, layout, callToAction
9777d01
@MonkeyDo MonkeyDo changed the title migrate react-fontawesome to offical package- BB358 migrate react-fontawesome to offical package- BB-358 Dec 2, 2019
@MonkeyDo MonkeyDo merged commit 2b183c6 into metabrainz:master Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MonkeyDo MonkeyDo MonkeyDo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@sbvkrishna @coveralls @MonkeyDo