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.

Sign up for GitHub

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

Jump to bottom

display network notices #94

Merged
merged 3 commits into from
Sep 19, 2023
Merged

display network notices #94

merged 3 commits into from
Sep 19, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Sep 18, 2023

Reads a notices list from https://main.agoric.net/network-config and displays them as toast to the user.

Also has a yarn upgrade for deps hygiene.

Test plan

Added this before the toast line to simulate data coming from network-config,

      config.notices = [
        {
          start: '2020-01-01',
          end: '2040-01-01',
          message:
            'Mainnet will be unavailable for up to a minute during a chain upgrade at 2023-09-21T19:37Z',
        },
      ];

toast demo

@turadg turadg mentioned this pull request Sep 18, 2023
Open
@cloudflare-pages
Copy link

cloudflare-pages bot commented Sep 18, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4ae6369
Status: ✅  Deploy successful!
Preview URL: https://b20c5bf0.dapp-psm.pages.dev
Branch Preview URL: https://ta-notices.dapp-psm.pages.dev

View logs

samsiegart
samsiegart reviewed Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

Was the yarn.lock change intentional?

src/utils/networkConfig.ts
type MinimalNetworkConfig = {
rpcAddrs: string[];
chainName: string;
notices?: NetworkNotice[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to have separate notices for PSM and Vaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we do I think that has to go somewhere else. this is config for the network. appropriate for things like pending upgrades.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense

src/components/ChainConnection.tsx Outdated
});
for (const message of activeNotices(config)) {
toast.warn(message, { position: 'top-center' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to support multiple concurrent notices? It would be be cleaner functionally and aesthetically to reuse https://github.com/Agoric/dapp-psm/blob/main/src/components/AnnouncementBanner.tsx and put this logic in there. If there's multiple maybe we can rethink the design, because having a bunch of toasts pop up isn't ideal either. Not sure the priority of this though, if we have time to think it through more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I figured there might be one or two max. And I didn't want to risk having someone defined multiple messages and one not go out due to client implemenation. (We don't have any validation tools on the production side.)

I think AnnouncementBanner makes sense for announcements, which can be delivered one a time and dismissed. "Ok, I saw it, got it." Whereas "notice" is more aggressive like, "DO NOT FORGET

Copy link
Collaborator

Choose a reason for hiding this comment

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

The announcement banner is what we've used for notices such as chain upgrades in the past. For multiple concurrent notices, I'm thinking it's sufficiently uncommon that showing multiple banners, or multiple rows in the banner, would be fine in that case. I think it should also show when the dapp loads, before the user has to connect their wallet. By doing it that way, the PR won't reinvent any of our existing UX, just make it more convenient to update the notice.

test/utils/networkConfig.test.ts
@@ -0,0 +1,40 @@
import { expect, it, describe } from 'vitest';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay tests

@turadg
Copy link
Member Author

turadg commented Sep 18, 2023 •
edited
Loading

Was the yarn.lock change intentional?

Yeah, called out in the PR description and separated with a commit.

If you want it in a different PR lemme know. But I figure they're going to deploy together anyway.

samsiegart
samsiegart previously requested changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

Yeah, called out in the PR description and separated with a commit.

Ah okay thanks

@samsiegart
Copy link
Collaborator

Updated to use the banner
image

turadg
turadg commented Sep 19, 2023
View reviewed changes
Copy link
Member Author

@turadg turadg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I opened the PR so I can't approve. I wish GH allowed transferring a PR.

I'm adding automerge so it'll land once you approve. Automerge can't be set until there's an approval. So just Approve and land at will.

@turadg turadg dismissed samsiegart’s stale review September 19, 2023 16:33

Sam addressed his requested changes

@samsiegart samsiegart merged commit d7d3cd9 into main Sep 19, 2023
2 checks passed
@samsiegart samsiegart deleted the ta/notices branch September 19, 2023 17:02
@otoole-brendan otoole-brendan mentioned this pull request Sep 28, 2023
Closed
This was referenced May 29, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsiegart samsiegart samsiegart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@turadg @samsiegart