Paragon StatusAlert deprecation

[abdullahwaheed]

Ticket

Migrate off deprecated Paragon components

What has changed

Removed deprecated StatusAlert component of paragon and updated it with Alert
Updated Paragon package to use ZoomOut icon in place of faSearchMinus of font-awesome

References

Paragon StatusAlert
Paragon Alert

[davidjoy]

This seems like a fine change - the owning team should also review so they're aware of the change, it should be tested by Fed-BOM and we can get it in. I did a cursory review.

[christopappas]

Pulled this down locally to test. On the search results page, before this change I'm seeing alerts render like:

after this change they render as:

I'll ask internally, but I'm not sure if this is an oddity with my local setup or a new and intended design

[abdullahwaheed]

Pulled this down locally to test. On the search results page, before this change I'm seeing alerts render like:

after this change they render as:

I'll ask internally, but I'm not sure if this is an oddity with my local setup or a new and intended design

are you using edx theme?

[christopappas]

@abdullahwaheed i was running locally with npm run start:with-theme

[adamstankiewicz]

For what it's worth, I don't think this is a theme issue. At least for the screenshot example Chris shared, that was largely that custom styles (CSS utility classes) overriding core Paragon styles. A similar style issue would happen with either the core Open edX theme or edX.org theme.

@abdullahwaheed It would be helpful to include screenshots of each Alert that was changed in this PR to help make the review more efficient. Generally, you'd want to include screenshots for any UI change.

[abdullahwaheed]

Yes this wan't theme issue. I faced similar issue in openedx/credentials repo that's why guessed its a theme issue. Fixed it now

[abdullahwaheed]

I had to update Paragon package to use ZoomOut icon in place of faSearchMinus of font-awesome

[abdullahwaheed]

@adamstankiewicz i have to revert back Paragon version as it was breaking other changes. That's why i have used FontAwesome icons for legacy view

[abdullahwaheed]

hey @christopappas, i have updated it with theme changes, could you please review it so we can merge it

[adamstankiewicz]

i have to revert back Paragon version as it was breaking other changes.

@abdullahwaheed May I ask what the later version of Paragon was breaking? 👀 Thanks for attempting to try to move away from the FontAwesome icons, anyways!

[abdullahwaheed]

i have to revert back Paragon version as it was breaking other changes.

@abdullahwaheed May I ask what the later version of Paragon was breaking? 👀 Thanks for attempting to try to move away from the FontAwesome icons, anyways!

this was the error due to upgrade
https://github.com/openedx/frontend-app-learner-portal-enterprise/runs/7522700089?check_suite_focus=true#step:6:3639

[adamstankiewicz]

@abdullahwaheed

this was the error due to upgrade
https://github.com/openedx/frontend-app-learner-portal-enterprise/runs/7522700089?check_suite_focus=true#step:6:3639

Are you specifically referring to ReferenceError: ResizeObserver is not defined?

I don't think that error is reason to not continue upgrading Paragon and move away from FontAwesome. Jest uses JSDom which doesn't support ResizeObserver; instead, we could mock ResizeObserver in the tests like we do in a few existing places:

• https://github.com/openedx/paragon/blob/81cae2e716f5fe0b40f57216487fcd0db8d3d487/src/setupTest.js#L6-L20
• https://github.com/openedx/frontend-app-admin-portal/blob/f6600f101db06d2aa7199db6c5fbc5a868c24779/src/setupTest.js#L40

The latter example uses a package called resize-observer-polyfill to handle the mock.

[abdullahwaheed]

@adamstankiewicz i have upgraded paragon and removed the use of FontAwesome icons as you have suggested. But this app still uses FontAwesomeIcon at multiple places other than StatusAlert. We have to create a separate ticket for that purpose

[codecov]

Codecov Report

Base: 79.09% // Head: 81.72% // Increases project coverage by +2.62% 🎉

Coverage data is based on head (f67e0df) compared to base (8076999).
Patch coverage: 79.82% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
+ Coverage   79.09%   81.72%   +2.62%     
==========================================
  Files         266      272       +6     
  Lines        5229     5385     +156     
  Branches     1276     1295      +19     
==========================================
+ Hits         4136     4401     +265     
+ Misses       1067      959     -108     
+ Partials       26       25       -1     

Impacted Files	Coverage Δ
src/components/TagCloud/index.jsx	100.00% <ø> (ø)
src/components/app/App.jsx	0.00% <0.00%> (ø)
src/components/app/EnterpriseAppPageRoutes.jsx	0.00% <0.00%> (ø)
src/components/course/CourseRunCard.jsx	96.77% <ø> (ø)
...mponents/dashboard/SubscriptionExpirationModal.jsx	92.68% <ø> (ø)
...n-content/course-enrollments/CourseEnrollments.jsx	95.00% <ø> (ø)
.../main-content/course-enrollments/CourseSection.jsx	96.07% <ø> (ø)
...course-cards/email-settings/EmailSettingsModal.jsx	93.75% <ø> (ø)
...omponents/pathway-progress/PathwayProgressCard.jsx	100.00% <ø> (ø)
...athway-progress/PathwayProgressContextProvider.jsx	0.00% <0.00%> (ø)
... and 95 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[arbrandes]

Are we good to merge, here? If so, I'll press the button.