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

Fixing the ./examples/src/App.jsx issues #277

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

Soham1803
Copy link

  • Installed the imported packages in ./examples/src/App.jsx issues viz: wrouter & use-error-boundary.
  • Configured the eslintrc file to remove the following parsing error.
    parsing-error-reactxr

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 6, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 18becbe:

Sandbox Source
examples Configuration

@Soham1803
Copy link
Author

There are still 2 warnings:
image
These could be ignored, but I wonder whether should be fixed or not?

@saitonakamura
Copy link
Collaborator

Hello @Soham1803 , thank you for the contribution! It's been a long-standing issue and it's great to see someone tackle it!

I've taken deliberation of fixing some stuff

  • I've removed dependencies added to lib package.json. Problem was, you needed to run yarn install in examples directory to work. We don't have a Contributing guide now, so I understand it wasn't clear. Can I ask your feedback for docs: contributing guide #278? Right now it's a barebone version and newcomer contributor feedback would be appreciated
  • I fixed those 2 warnings in examples/src/App.tsx
  • I've added linting examples to yarn lint so now CI will also warn about errors in examples

Let me know if it works for you in current state

@Soham1803
Copy link
Author

  • In docs, would it be a good idea to mention npm commands too, besides some developers might would directly refer to the "scripts" in package.json to run the commands.
  • Examples run fine on my local device.
  • Should the packages: wouter & use-error-boundary uninstalled from lib node_modules?

@saitonakamura
Copy link
Collaborator

In docs, would it be a good idea to mention npm commands too, besides some developers might would directly refer to the "scripts" in package.json to run the commands.

I've added some of the scripts that will be most useful for the contributing process here https://github.com/pmndrs/react-xr/pull/278/files

Should the packages: wouter & use-error-boundary uninstalled from lib node_modules?

For you, locally, yes. You'll just need to run yarn in the repo root for this

@Soham1803
Copy link
Author

I've added some of the scripts that will be most useful for the contributing process here https://github.com/pmndrs/react-xr/pull/278/files

Awesome, the doc looks great, with all the essential scripts mentioned.

For you, locally, yes. You'll just need to run yarn in the repo root for this

Fine, got it. Thanks by the way!

I think we can proceed then...

@saitonakamura saitonakamura merged commit b3f466d into pmndrs:master Jun 8, 2023
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.

2 participants