-
Notifications
You must be signed in to change notification settings - Fork 1
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
MPDX- 8231 - Announcements #1162
Conversation
Is there no Material icons that can substitute for these FA icons? It seems like a step backwards to re-introduce those icons. Is this the free or paid version of FA? |
It's a free version. I can look into seeing if we can get MUI to work, but I was trying to avoid having to update all the announcement icon actions, as they are They all look like they are using the
|
Ah, I assumed since the whole site was using Material icons already that we would have already loaded them. |
If we're using the free version of FA here, it is less of a concern. One of the benefits to moving to Material/React was dropping the dependency on a paid license of FA, in particular for non-Cru orgs. |
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.
This looks great! I'll have to spend some time testing it, but the code and tests look really well-written and organized!
src/components/Announcements/AnnouncementAction/AnnouncementAction.tsx
Outdated
Show resolved
Hide resolved
src/components/Announcements/AnnouncementBanner/AnnouncementBanner.tsx
Outdated
Show resolved
Hide resolved
src/components/Announcements/AnnouncementAction/AnnouncementAction.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Announcements/AnnouncementModal/AnnouncementModal.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Announcements/AnnouncementModal/AnnouncementModal.test.tsx
Show resolved
Hide resolved
@wrandall22 @canac should we use Material Icons instead of Font-Awesome? I'm okay with switching it. I guess one of the advantages of using material icons, is that it's in the MUI family, the only down side is we would have to update 337 rows in the DB, but this shouldn't take long. |
@dr-bizz It seems cleaner to just use Material Icons since that's what we're already using in this project. For the migration, do you have to create a mapping of icon names from MUI to FA? That kind of migration feels difficult. Also, will it be harder to make the MUI icons show up since it's an import instead of just adding the right classname? |
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.
I couldn't find any issues during testing. Great work!
FYI, to undismiss announcements, I connected to the staging database and ran delete from "person_announcements" where "announcement_id" = 'xxx' and "user_id" = 'xxx';
It worked really well.
This is great to know. It's weird that they used the word "person" instead of user. I think that is what threw me off finding the table. I've messaged in slack about how we can use Material Icons |
Preview branch generated at https://MPDX-8231.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 9d15d74
|
…E if it's on a page without apollo (login screen.)
…times. I also updated a colour on the new MPDX as it was slightly off from the MPDX logo
…ing "show" class to banner.
…nd adding better support for admin entered GO arguments.
Description
In this PR, I add the announcements to MPDX. There are two versions of announcements,
Banners
andModals
; I've added both.You will also see that I have moved the
<AlertBanner/>
into the announcements component; this is because we don't want both showing at the same time, and we need to have an authenticated session to use GraphQL, where AlertBanner does not, meaning we can show an alert on all pages if we wanted to. AlertBanner will primarily be used to show an alert on the login page.You will see that I also have to use font awesome, but as we only want it for this component, I load it just after the initial render and then remove it on
unmount
.How to Test
The banners and modals should only show announcements that are in your language. See PR here: https://github.com/CruGlobal/mpdx_api/pull/2875.
I found it easier to test on local with a local version of the API running, as I can alter the DB quickly, but test as you prefer.
You will need to alter an existing announcement (which you haven't acknowledged) or create a new one.
If you need additional reference of what it should look like, you can go to old.stage-mpdx.org to see how it used to look.
Banner UI
The image doesn't show on the banner.
Modal UI
Announcement Actions
Go
action should redirect the user to another page. And then acknowledge the announcement so it doesn't show again.AppealCreate
action should open the create appeal modal, prefill details about the end-of-year appeal, and acknowledge the announcement.Track
action shoulddispatch
the announcement args to analytics and then acknowledge the announcement.This PR will fail until I merge the API PR into production https://github.com/CruGlobal/mpdx_api/pull/2875.
Checklist: