In-app Announcement popup #2318
Conversation
src/shell/services/announcements.ts
Outdated
|
|
||
| export const announcementsApi = createApi({ | ||
| reducerPath: "announcementsApi", | ||
| baseQuery: fetchBaseQuery({ baseUrl: "https://www.zesty.io/-/instant/" }), |
There was a problem hiding this comment.
This base url seems very generic yet it is supporting a specific service.
Let's make this a more general service and tie this less with announcements.
If we want we can keep the getAnouncment query to maintain response structure but it should have hard coded values to announcement model
There was a problem hiding this comment.
Updated. Renamed service to marketingApi so that it's more generic towards any marketing-related queries. Also made the query have the model zuid by default instead of passing it as a prop
| justifyContent="center" | ||
| boxSizing="border-box" | ||
| > | ||
| <Box |
There was a problem hiding this comment.
Why not have this be an a tag and avoid the onClick handler?
| @@ -44,3 +44,12 @@ Cypress.Commands.add("assertClipboardValue", (value) => { | |||
| Cypress.Commands.add("getBySelector", (selector, ...args) => { | |||
| return cy.get(`[data-cy=${selector}]`, ...args); | |||
| }); | |||
|
|
|||
| Cypress.Commands.add("blockAnnouncements", () => { | |||
There was a problem hiding this comment.
This seems like it would need to be applied to every test in order to avoid it interrupting them. Can we apply it by default and then manual exclude if needing to test the announcement behavior? This way we don't have to remember to add it for every new test.
| <Box | ||
| component="img" | ||
| alt="announcement-banner-image" | ||
| src={announcementData?.feature_image?.data[0]?.url ?? ""} |
There was a problem hiding this comment.
This URL should contain width and height parameters that match the rendered size.
| boxSizing="border-box" | ||
| > | ||
| <Box | ||
| component="img" |
There was a problem hiding this comment.
This element should contain width and height attributes that match the image dimensions
There was a problem hiding this comment.
Not sure if this comment may help and I could be wrong. The way we have configured it, all aspect ratios and sizes will always be respected no matter what image resolution is uploaded. I have already tested it for the same. We are using image contain fit.
There was a problem hiding this comment.
Its not a concern of ratio but of performance
|
|
||
| export const marketingApi = createApi({ | ||
| reducerPath: "marketingApi", | ||
| baseQuery: fetchBaseQuery({ baseUrl: "https://www.zesty.io/" }), |
There was a problem hiding this comment.
This needs to be a configuration value that can be different within different environments
There was a problem hiding this comment.
@shrunyan are you suggesting that this should be pointing to the dev env of the marketing instance if manager-ui is being ran on the dev env, same for stage?
| endpoints: (builder) => ({ | ||
| // Get announcements via instant api from the marketing instance | ||
| getAnnouncements: builder.query<Announcement[], void>({ | ||
| query: () => "/-/instant/6-90fbdcadfc-4lc0s5.json", |
There was a problem hiding this comment.
This needs to be a configuration value that can be different within different environments
There was a problem hiding this comment.
This ones debatable on whether it should be a config value
|
|
||
| const unreadAnnouncements = useMemo(() => { | ||
| if (announcements?.length) { | ||
| return announcements |
There was a problem hiding this comment.
Currently if there are multiple announcements in the publish range the pop-up will cycle through them as you ignore them. It should only show the single latest announcement.
There was a problem hiding this comment.
@shrunyan oohh, I thought it was supposed to show everything starting from the latest announcement first.
|
|
||
| export const InAppAnnouncement = () => { | ||
| const [activeAnnouncementIndex, setActiveAnnouncementIndex] = useState(0); | ||
| const [readAnnouncements, setReadAnnouncements] = useLocalStorage( |
There was a problem hiding this comment.
Local storage isolates data by origin. This is causing the pop-up to show on ever instance a user visits. We want to avoid that and only show the announcement once per user. 2 alternatives I can think of are using a cookie or storing it to the users account
There was a problem hiding this comment.
Closes #2306
Preview