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

Scaffold: 1 – Initial set-up #1

Merged
merged 5 commits into from
Aug 5, 2020
Merged

Scaffold: 1 – Initial set-up #1

merged 5 commits into from
Aug 5, 2020

Conversation

andy-hook
Copy link
Contributor

@andy-hook andy-hook commented Aug 3, 2020

Initial configuration and essentials. Using Parcel because that's tried and tested, perhaps in the future we can use snowpack.

Copy link
Contributor Author

@andy-hook andy-hook left a comment

Choose a reason for hiding this comment

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

@sohkai – some of this config it likely redundant, I wanted to raise it here first so we could discuss. I'm on board with starting with the smallest amount and adding as we go.

.babelrc Show resolved Hide resolved
now-mainnet.json Outdated Show resolved Hide resolved
scripts/start Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

Looking good! I left a few comments, let me know what you think 🤗

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
.github/workflows/mainnet.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/network-config.js Outdated Show resolved Hide resolved
}

const networkConfig = getNetworkConfig(networkType)
export const network = networkConfig.settings
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be nice to make the network dynamic from the start (maybe as a hook?).

Should we also start from the changes made in Brett’s PR? aragon/client#1485

manifest.json Outdated Show resolved Hide resolved
@andy-hook andy-hook marked this pull request as draft August 3, 2020 13:35
@andy-hook
Copy link
Contributor Author

Thanks @bpierre ! I didn’t change anything from the client thinking we wanted to port what we have as quick as possible. I’d love to freshen it all up though and since you agree I’ll go and make the changes! The only thing I’d worry about right now is framer motion, we’ve already written a lot of spring and it might not be worth the time to transition at this particular moment.

@vercel vercel bot requested a deployment to Preview August 3, 2020 14:11 Abandoned
@vercel vercel bot requested a deployment to Preview August 3, 2020 14:11 Abandoned
@vercel vercel bot requested a deployment to Preview August 3, 2020 14:11 Abandoned
@vercel vercel bot requested a deployment to Preview August 3, 2020 14:12 Abandoned
@bpierre
Copy link
Contributor

bpierre commented Aug 3, 2020

The only thing I’d worry about right now is framer motion, we’ve already written a lot of spring and it might not be worth the time to transition at this particular moment.

Sounds good! Let’s delay the move to Framer Motion 👍

@andy-hook andy-hook closed this Aug 3, 2020
@andy-hook andy-hook changed the title Initial scaffold Scaffold: Initial set-up Aug 4, 2020
@andy-hook andy-hook reopened this Aug 4, 2020
@andy-hook andy-hook marked this pull request as ready for review August 4, 2020 09:39
now-mainnet.json Outdated
Comment on lines 8 to 10
"FORTMATIC_API_KEY": "@aragon-client-fortmatic-api-key",
"PORTIS_DAPP_ID": "@aragon-client-portis-dapp-id",
"SENTRY_DSN": "@aragon-client-sentry-dsn"
Copy link
Contributor Author

@andy-hook andy-hook Aug 4, 2020

Choose a reason for hiding this comment

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

Leaving these as the client secrets for now just to get the deploys working, they will be updated to use scoped env vars via Vercel in a separate improvement.

@andy-hook
Copy link
Contributor Author

andy-hook commented Aug 4, 2020

@bpierre @facuspagnuolo – This is ready for review. Now that I know we don't need all of the client baggage I stripped down everything and refreshed the dependencies. Network configuration, routing etc will come in separate improvements.

Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

LGTM! Just left a few comments but we could make these changes separately.

.eslintrc.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Comment on lines +12 to +13
<link rel="icon" type="image/svg+xml" href="icons/favicon.svg" />
<link rel="icon" type="image/png" href="icons/favicon.png" />
Copy link
Contributor

Choose a reason for hiding this comment

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

(Not necessarily in this PR)

We might want to use a higher size than 33x33 for the PNG, maybe 128x128? The SVG should be fine at any size, but it could happen that the SVG size is used to produce the raster image at the same size, so I think we could use 128x128 for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Icon compat is a bit of a minefield :( but this is a good suggestion! Adri is adjusting some graphics too so i'll do them at the same time separately :)

src/index.js Outdated Show resolved Hide resolved
Co-authored-by: Pierre Bertet <hello@pierre.world>
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

✅✅✅

.github/workflows/ci.yml Show resolved Hide resolved
now-mainnet.json Outdated Show resolved Hide resolved
now.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/environment.js Outdated Show resolved Hide resolved
src/environment.js Show resolved Hide resolved
src/environment.js Outdated Show resolved Hide resolved
src/environment.js Outdated Show resolved Hide resolved
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.

3 participants