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

Setup nextjs demo page #183

Merged
merged 2 commits into from
Dec 31, 2021
Merged

Setup nextjs demo page #183

merged 2 commits into from
Dec 31, 2021

Conversation

etr2460
Copy link
Contributor

@etr2460 etr2460 commented Dec 29, 2021

Closes #182

Description

Fixes a few issues with the next js demo setup and adds a very basic demo page that verifies components work. Also converts everything to using TS

📝 Additional Information

There's probably still more work to be done to make a good demo page. I'm also a bit uncertain if a demo page is necessary in addition to the storybook. Food for thought

Screenshot

image

@changeset-bot
Copy link

changeset-bot bot commented Dec 29, 2021

⚠️ No Changeset found

Latest commit: 8bc50fa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@etr2460 etr2460 requested a review from Dhaiwat10 December 29, 2021 00:02
@with-heart
Copy link
Member

I'm also a bit uncertain if a demo page is necessary in addition to the storybook. Food for thought

Yeah, I think you're right to be uncertain there. imo the example should become a real-world app, something that gives us a way to demonstrate realistic use cases for each piece of web3-ui (and show off our creativity too! 😁).

Copy link
Member

@with-heart with-heart left a comment

Choose a reason for hiding this comment

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

Can you remove example/package-lock.json and make sure it's working as a workspace with yarn?

@Dhaiwat10
Copy link
Member

imo the example should become a real-world app, something that gives us a way to demonstrate realistic use cases for each piece of web3-ui

That's precisely what I was thinking. The Storybook is great at showcasing the components and hooks in isolation, but something that people can play around with directly (we will deploy this example to Vercel or sth) and maybe clone the code and try it out for themselves would be nice too. Maybe this can even turn into a starter template with our package all setup and ready-to-go, npx create-web3-ui-app, who knows?

Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

Just one tiny change. I'll do this myself, push and merge :) Thanks @etr2460!

@@ -3,7 +3,7 @@ import { Provider } from '@web3-ui/core';

function MyApp({ Component, pageProps }) {
return (
<Provider network={4}>
<Provider network={1}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Provider network={1}>
<Provider network={NETWORKS.mainnet}>

More readable. NETWORKS is exported from core and hooks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Dhaiwat10
Copy link
Member

Oh I missed @with-heart's review. Will also try to have a look at that.

@Dhaiwat10
Copy link
Member

image

I can't run this locally. I'm running yarn example from root. Anyone else facing the same problem?

@etr2460
Copy link
Contributor Author

etr2460 commented Dec 29, 2021

Can you remove example/package-lock.json and make sure it's working as a workspace with yarn?

Cross posting from discord, the examples directory was initially setup with npm, not yarn. i'm not sure if that was intentional. Maybe there's some questions here to be answered first:

  • Should we have an example site, or is the storybook sufficient?
  • Should the example use npm or yarn?

Once we answer both of these, we can decide if this PR should be updated, or if we should simply delete the examples directory for now

@Dhaiwat10
Copy link
Member

Should we have an example site, or is the storybook sufficient?

I think we do need an example because of the reasons I mentioned here. Tldr; storybook is good for showcasing stuff in isolation but we need an real-world 'app' example where people can see how all of that stuff actually works together.

Should the example use npm or yarn?

It should use yarn because that's what we are using everywhere else in the project. The package-lock.json was unintentional. Sorry for that.

@etr2460
Copy link
Contributor Author

etr2460 commented Dec 31, 2021

@Dhaiwat10, @with-heart this should be g2g now

@@ -24,7 +24,7 @@
"test:coverage": "jest --coverage --colors",
"storybook": "start-storybook -p 9001 -s ./packages/components/src/assets -c .storybook",
"postinstall": "preconstruct dev",
"example": "yarn workspace example dev",
"example": "yarn build && yarn workspace example dev",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for nextjs to load the packages, we need to yarn build them first. This isn't technically correct as the examples should depend on a version of the packages and not the main branch of them, but I couldn't for the life of me figure out how to get yarn to pull in the packages from npm instead of symlinks. Maybe there was a caching issue, but this seems good enough for me

@Dhaiwat10
Copy link
Member

Way to go @etr2460 ❤️

@Dhaiwat10 Dhaiwat10 merged commit 3db211f into main Dec 31, 2021
@Dhaiwat10 Dhaiwat10 deleted the setup-nextjs-demo branch December 31, 2021 10:21
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.

Setup a Next.js example
3 participants