feat: added banner for network-config notices #104

frazarshad · 2024-05-29T13:22:22Z

This PR adds the NoticeBanner in @agoric/react-components

cloudflare-workers-and-pages · 2024-05-30T09:27:56Z

Deploying dapp-econ-gov with Cloudflare Pages

Latest commit:	`e6d9ee4`
Status:	✅ Deploy successful!
Preview URL:	https://b81ff2c2.dapp-econ-gov.pages.dev
Branch Preview URL:	https://feat-add-notice-banner.dapp-econ-gov.pages.dev

View logs

frazarshad · 2024-05-31T13:43:17Z

The branch was having issues creating a build.
When tested on my machine, the node process ran out of memory, when i allotted more memory to it, the build passed but the chunk size has increased drastically
@samsiegart

github-actions · 2024-05-31T18:43:15Z

Cloudflare deployment logs are available here

samsiegart

I see there's no issue, I'm just wondering so I can understand the goals better, is there somewhere where this is written down like a doc or something? Most PRs especially features should have issues.

The CF build failed because react-components requires node 18 and it was set up to use 16. I just changed the setting and it builds successfully. However, the test workflow is failing, you just need to run prettier with yarn format (or set up your IDE to run prettier automatically on-save).

The bundle size isn't great. That's why I thought importing react-components might be heavy-handed. How did you allot more memory to the node process? In the UI tutorial it suggests to do so with max-old-space-size. But, it's easier to justify when an app is making full use of the AgoricProvider and leap elements. For this I'm tempted to suggest just copying the component into this project because it's pretty simple. That's why I was wondering about an issue, because if the only goal was to add a banner to this dapp I think that approach would've been fine.

samsiegart · 2024-05-31T18:03:03Z

tailwind.config.cjs

+  content: [
+    './src/**/*.{js,jsx,ts,tsx}',
+    './public/index.html',
+    './node_modules/@agoric/react-components/**/*.{ts,tsx,mjs}',


We don't want to do this. It only works because there's no custom variables or themes in the banner component, so the default tailwind stuff provided in this project's config happens to work.

It should be:

import '@agoric/react-components/dist/style.css';

in src/App.tsx, as documented in https://github.com/Agoric/ui-kit/tree/main/packages/react-components#integrating

samsiegart · 2024-05-31T18:13:57Z

src/config.ts

@@ -50,3 +50,30 @@ export const rpcUrl = agoricNetSubdomain =>
 */
 export const archivingAlternative = (chainName: string, defaultRpc: string) =>
  chainName === 'agoric-3' ? 'https://main-a.rpc.agoric.net:443' : defaultRpc;
+
+export const networkConfigs = {


This should be unnecessary after addressing the other comments

samsiegart · 2024-05-31T18:24:44Z

src/utils/networkConfig.ts

+  notices?: NetworkNotice[];
+};
+
+export const loadNetworkConfig = (url: string): Promise<MinimalNetworkConfig> =>


We already load the network config in suggestChain. I'd suggest moving that fetch into makeChainKit before suggestChain is called, and changing suggestChain to accept the loaded network config instead of doing the fetch itself. Then, add the loaded network config to the return value of makeChainKit.

samsiegart · 2024-05-31T18:26:45Z

src/utils/networkConfig.ts

@@ -0,0 +1,16 @@
+type NetworkNotice = {


These types should be imported from @agoric/react-components

samsiegart · 2024-05-31T18:30:46Z

src/App.tsx

@@ -79,8 +82,11 @@ const App = (_props: Props) => {

  const address = walletUtils.getWalletAddress();

+  const config = useAtomValue(loadable(networkConfigPAtom));


This new atom should be unnecessary with the other comments, the config should be accessible from:

const { chainKit } = useContext(WalletContext); const config = chainKit.networkConfig; // Assuming you add this to chainKit as mentioned

samsiegart · 2024-05-31T18:31:26Z

src/store/app.ts

@@ -32,3 +34,12 @@ export const displayFunctionsAtom = atom(get => {

 /**  Experimental feature flag. */
 export const previewEnabledAtom = atom(_get => false);
+
+const networkConfigAtom = atomWithStorage(


These new atoms should be unnecessary with other related suggestions

chore: reset yarn.lock to match main branch

frazarshad · 2024-06-03T13:40:45Z

After @samsiegart's comments. made the following updates to the PR:

removed dependancy on react-components since it drastically increased the build size
used networkConfig that was already being fetched in chainKit

samsiegart

Looks great thanks!

frazarshad mentioned this pull request May 29, 2024

feat: added network notice banner as a resuable component Agoric/ui-kit#112

Merged

feat: added banner for notices

0869749

frazarshad force-pushed the feat/add-notice-banner branch from 43116b0 to 0869749 Compare May 31, 2024 12:17

frazarshad requested a review from samsiegart May 31, 2024 13:43

frazarshad marked this pull request as ready for review May 31, 2024 13:43

samsiegart requested changes May 31, 2024

View reviewed changes

frazarshad added 4 commits June 3, 2024 17:48

refactor: remove dependancy on react-components

adad25d

refactor: fetching networkConfig from chainKit instead of atom

de7ddad

fixup! refactor: remove dependancy on react-components

b18a9f5

chore: reset yarn.lock to match main branch

chore: prettier update

e6d9ee4

frazarshad requested a review from samsiegart June 3, 2024 13:40

samsiegart approved these changes Jun 3, 2024

View reviewed changes

frazarshad merged commit 0f710bd into main Jun 4, 2024
3 checks passed

frazarshad deleted the feat/add-notice-banner branch June 4, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added banner for network-config notices #104

feat: added banner for network-config notices #104

frazarshad commented May 29, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 30, 2024 •

edited

Loading

frazarshad commented May 31, 2024

github-actions bot commented May 31, 2024 •

edited

Loading

samsiegart left a comment

samsiegart May 31, 2024

samsiegart May 31, 2024

samsiegart May 31, 2024

samsiegart May 31, 2024

samsiegart May 31, 2024 •

edited

Loading

samsiegart May 31, 2024 •

edited

Loading

frazarshad commented Jun 3, 2024

samsiegart left a comment

		@@ -79,8 +82,11 @@ const App = (_props: Props) => {

		const address = walletUtils.getWalletAddress();

		const config = useAtomValue(loadable(networkConfigPAtom));

feat: added banner for network-config notices #104

feat: added banner for network-config notices #104

Conversation

frazarshad commented May 29, 2024 • edited Loading

cloudflare-workers-and-pages bot commented May 30, 2024 • edited Loading

Deploying dapp-econ-gov with Cloudflare Pages

frazarshad commented May 31, 2024

github-actions bot commented May 31, 2024 • edited Loading

samsiegart left a comment

Choose a reason for hiding this comment

samsiegart May 31, 2024

Choose a reason for hiding this comment

samsiegart May 31, 2024

Choose a reason for hiding this comment

samsiegart May 31, 2024

Choose a reason for hiding this comment

samsiegart May 31, 2024

Choose a reason for hiding this comment

samsiegart May 31, 2024 • edited Loading

Choose a reason for hiding this comment

samsiegart May 31, 2024 • edited Loading

Choose a reason for hiding this comment

frazarshad commented Jun 3, 2024

samsiegart left a comment

Choose a reason for hiding this comment

frazarshad commented May 29, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 30, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

samsiegart May 31, 2024 •

edited

Loading

samsiegart May 31, 2024 •

edited

Loading