feat: add sitewide banner to publisher #993
Conversation
AfaqShuaib09
commented
Dec 19, 2024
•
AfaqShuaib09
commented
AfaqShuaib09
force-pushed
the
afaq/prod-4263
branch
from
9ee48a7 to
ef34879
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #993 +/- ##
==========================================
+ Coverage 67.77% 67.90% +0.13%
==========================================
Files 128 129 +1
Lines 3230 3247 +17
Branches 936 941 +5
==========================================
+ Hits 2189 2205 +16
- Misses 993 994 +1
Partials 48 48 ☔ View full report in Codecov by Sentry. |
src/containers/MainApp/index.jsx
Outdated
| message={process.env.SITEWIDE_BANNER_CONTENT || ''} | ||
| type="primary" | ||
| dismissible | ||
| cookieName="sitewidebannerDismissed" |
There was a problem hiding this comment.
nit: please make the cookie name unique enough, such as publisherSiteWideBannerViewed or publisherSiteWideBannerDismissed
There was a problem hiding this comment.
should we move it to edx-internal or keep it here?
| if (!isVisible) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
nit: instead of returning null here, we can use if-else format that has been used in other places in MFEs in opened. It makes the code consistent. This code looks a bit out of place.
src/containers/MainApp/index.jsx
Outdated
|
|
||
| const MainApp = () => ( | ||
| <div> | ||
| <Header /> | ||
| <SitewideBanner | ||
| message={process.env.SITEWIDE_BANNER_CONTENT || ''} | ||
| type="primary" |
There was a problem hiding this comment.
should it be primary or warning?
AfaqShuaib09
force-pushed
the
afaq/prod-4263
branch
from
ab0a8a1 to
2e0f969
Compare
AfaqShuaib09
force-pushed
the
afaq/prod-4263
branch
from
16d6007 to
6b6ab33
Compare
