Skip to content

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Mar 16, 2023

Description

Added a scaffold.config.ts in frontend instead for now as discussed in #231.

Updated network.ts now it uses block explores links from targetNetowork and now you get transaction link for different blockexplores previously it used only show for etherscan and dai.... its the same as #231 (comment)

would love to make it more robust and inline in different PR 🙌

Flow :

  1. targetNetwork from scaffold.config.ts is single source of truth now for frontend
  2. Removed .env.* and added them inside scaffold.config.ts

@technophile-04 technophile-04 requested review from rin-st and sverps March 16, 2023 18:56
@carletex
Copy link
Member

Hey @technophile-04 great job!

Left a few comments.

I think this should be the way to go, right? At least initially. (instead of #231 )

@technophile-04
Copy link
Collaborator Author

I think this should be the way to go, right? At least initially. (instead of #231 )

Yes for now 🙌, also added a small comment #231 (comment) there for the need of this PR

@sverps
Copy link
Collaborator

sverps commented Mar 17, 2023

One thing I'm not sure about: inlining scaffoldConfig.targetNetwork or scaffoldConfig.targetNetwork.id or scaffoldConfig.targetNetwork.name directly. In some cases I think I'd have done something like

It doesn't really bother me. It's a little more verbose, but at the same time it makes it obvious where the value is coming from (scaffold config), rather than abstracting this knowledge away.

Maybe we can improve it a bit by making the scaffold config a named export, and naming the const simply scaffold. That reduces verbosity, makes sure it's named the same all over for recognisability, and reads nice: scaffold.targetNetwork.id

@carletex carletex merged commit 2071f6e into main Mar 17, 2023
@carletex carletex deleted the feat/frontend-config branch March 17, 2023 16:19
@carletex carletex changed the title Use .config for frontend instaed of env's Use .config for frontend instead of env's Mar 17, 2023
@carletex
Copy link
Member

Great job everyone!

Merged this. Let's see how it feels, and don't forget about #231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants