-
Notifications
You must be signed in to change notification settings - Fork 31
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
chore: replace font-awesome with paragon icons #1017
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1017 +/- ##
==========================================
- Coverage 83.24% 83.21% -0.04%
==========================================
Files 400 400
Lines 8702 8697 -5
Branches 1797 1798 +1
==========================================
- Hits 7244 7237 -7
- Misses 1420 1422 +2
Partials 38 38
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nits.
src/components/Coupon/_Coupon.scss
Outdated
@@ -63,3 +63,7 @@ | |||
} | |||
} | |||
} | |||
|
|||
.align-right { | |||
display: inline-block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could use Paragon/Bootstrap's .d-inline-block
rather than defining a custom class name.
src/components/Coupon/index.jsx
Outdated
const iconColor = isExpanded ? 'text-white' : 'text-primary'; | ||
const screenReaderText = isExpanded ? 'Close' : 'Open'; | ||
return ( | ||
<Icon | ||
className={classNames('fa', iconClass, iconColor)} | ||
className={`align-right ${iconColor}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would recommend keeping the use of classNames
when working with dynamic class names.
.justify-left { | ||
margin-left: -40px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: bit wary of a seemingly-generic class name that is specific to this use case, especially when it's not namespaced. For example, I believe if some other page happen to start using .justify-left
because they think its a Bootstrap class name, these styles might start applying to them.
I would recommend namespacing .justify-left
under a class name specific to this use case, e.g. .reporting-config .justify-left
or similar.
…1003) * feat: ENT-7309 Added budget category-based page for learner credit Co-authored-by: IrfanUddinAhmad <irfanahmad@arbisoft.com> Co-authored-by: zamanafzal <zamanafzal@gmail.com>
6033732
to
a948f3a
Compare
Description
This is part of a larger effort to deprecate Font Awesome in favor of solely using Paragon Icons, which will reduce bundle size and improve performance (page load time) since it’s a larger library.
Solution
Replace all Font-awesome icons with Paragon icons. Before and after photos below:
ManageCodesTab.jsx
CodeSearchResultsHeading.jsx
src/components/Coupon/index.jsx
src/components/DownloadCsvButton/index.jsx
src/components/ReportingConfig/ReportingConfigForm.jsx
src/components/RequestCodesPage/RequestCodesForm.jsx
src/components/Sidebar/index.jsx
For all changes
Only if submitting a visual change