Skip to content
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

feat(protocol-designer): add announcement toast #16562

Merged
merged 5 commits into from
Oct 23, 2024
Merged

feat(protocol-designer): add announcement toast #16562

merged 5 commits into from
Oct 23, 2024

Conversation

koji
Copy link
Contributor

@koji koji commented Oct 22, 2024

Overview

add announcement toast

close AUTH-876

Test Plan and Hands on Testing

Changelog

  • add bakeToast to Landing page and add a test to Landing page

Review requests

Risk assessment

low

@koji koji requested review from ncdiehl11 and jerader October 22, 2024 16:57
@koji koji marked this pull request as ready for review October 22, 2024 16:58
@koji koji requested review from a team as code owners October 22, 2024 16:58
@koji koji removed request for a team October 22, 2024 16:59
@jerader
Copy link
Collaborator

jerader commented Oct 22, 2024

Screenshot 2024-10-22 at 13 17 54

I think we need to remove the modal from popping up in ProtocolRoutes

@jerader
Copy link
Collaborator

jerader commented Oct 22, 2024

also, a separate issue but the announcement modal needs to be updated to say 8.2.0 instead of 9.0.0

@@ -302,7 +302,7 @@ export const useAnnouncements = (): Announcement[] => {
),
},
{
announcementKey: 'redesign9.0',
announcementKey: process.env.OT_PD_VERSION ?? 'redesign9.0',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm i am reluctant to use the OT_PD_VERSION here because it is for an announcementKey. I know that we don't see previous announcements but i think it is sort of for legacy tracking of the previous versions.

can this instead be:

announcementKey: 'redesign8.2'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean changing that manually?

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the announcement modal is still rendering from ProtocolRoutes

Screenshot 2024-10-23 at 08 42 40

@koji
Copy link
Contributor Author

koji commented Oct 23, 2024

om ProtocolRoutes

it looks like the announcement modal is still rendering from ProtocolRoutes

Screenshot 2024-10-23 at 08 42 40

@jerader
you mean removing from the following?

 <>
      <NavigationBar />
      <Kitchen>
        <Box width="100%">
          <AnnouncementModal />
          <LabwareUploadModal />
          <FileUploadMessagesModal />
          <Routes>
            {allRoutes.map(({ Component, path }: RouteProps) => {
              return <Route key={path} path={path} element={<Component />} />
            })}
            <Route path="*" element={<Navigate to={landingPage.path} />} />
          </Routes>
        </Box>
      </Kitchen>
    </>

@jerader
Copy link
Collaborator

jerader commented Oct 23, 2024

om ProtocolRoutes

it looks like the announcement modal is still rendering from ProtocolRoutes
Screenshot 2024-10-23 at 08 42 40

@jerader you mean removing from the following?

 <>
      <NavigationBar />
      <Kitchen>
        <Box width="100%">
          <AnnouncementModal />
          <LabwareUploadModal />
          <FileUploadMessagesModal />
          <Routes>
            {allRoutes.map(({ Component, path }: RouteProps) => {
              return <Route key={path} path={path} element={<Component />} />
            })}
            <Route path="*" element={<Navigate to={landingPage.path} />} />
          </Routes>
        </Box>
      </Kitchen>
    </>

@koji oh, i think we just have to remove line 66: which is AnnouncementModal, basically what i tested on your branch, the modal double renders. which we don't want. it should only render once you press the button in the toast.

@koji koji requested a review from jerader October 23, 2024 15:31
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! good to merge after the checks are passing

@koji koji merged commit 57de055 into edge Oct 23, 2024
24 checks passed
@koji koji deleted the feat_AUTH-876 branch October 23, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants