-
Notifications
You must be signed in to change notification settings - Fork 96
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
fix: MailToLink to account for no emails #230
Conversation
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.
Overall this is looking great! I left a couple quick cleanup suggestions, but nothing major.
src/containers/CourseCard/components/CourseCardBanners/CertificateBanner.test.jsx
Outdated
Show resolved
Hide resolved
63be1e4
to
7390851
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #230 +/- ##
==========================================
- Coverage 96.46% 96.45% -0.01%
==========================================
Files 195 194 -1
Lines 1839 1835 -4
Branches 322 322
==========================================
- Hits 1774 1770 -4
Misses 60 60
Partials 5 5 ☔ View full report in Codecov by Sentry. |
@openedx/2u-aperture, can we get eyes on this one? Thanks! |
Oh, and @cintnguyen, mind rebasing? Thanks! |
7390851
to
b83f128
Compare
@arbrandes rebased, ci passed! |
I'm going to start getting eyes on this today but most likely won't complete review until tomorrow morning. Just wanted to acknowledge that it's being looked at. |
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.
Looks good to me! Thanks for your work on this.
Resolves #226
Removed EmailLink component and refactored code to cover for the use case of not having an email provided. This led to also creating additional tests to cover for these snapshots.