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

Add docs section in CONTRIBUTING.md #657

Merged
merged 4 commits into from
Dec 26, 2023

Conversation

zaaakher
Copy link
Contributor

Added a documentation section in the CONTRIBUTING.md file to guide people on how to run the docs locally to hopefully streamline the process of updating the docs when needed.

@zaaakher
Copy link
Contributor Author

I based the changes I made on the comment you made here

But it seems when I do yarn install and then yarn start from the root directory It doesn't run due to a missing embla-carousel-react (see image below)
image

I tried running yarn install inside /packages/embla-carousel-docs but I still get the same error when I run yarn start

@davidjerleke Are there additional steps I should follow to successfully run the docs locally?

@davidjerleke
Copy link
Owner

davidjerleke commented Dec 20, 2023

@zaaakher thank you for your contribution. This is a great start. You’re right. After yarn install you need to run yarn build before you run yarn start. I forgot to mention that!

I’ve updated the steps in the comment you linked to.

Best,
David

@zaaakher
Copy link
Contributor Author

@davidjerleke Thanks for the update, I tried running yarn build but it fails building embla-carousel-docs due to 3 errors in a row,

(I'm running node v18.17.0)

Error 1

image

Error 2

image

Error 3

image

@davidjerleke
Copy link
Owner

davidjerleke commented Dec 20, 2023

@zaaakher thanks for the additional details.

(I'm running node v18.17.0)

It's highly recommended to use nvm to easily install any node version you want alongside other versions. An important step is to use the correct node version for this project which is located in the .nvmrc file. At the time of writing it's v18.13.0. Try using the correct node version, run yarn install and yarn build again.

I definitely think that one of the steps in this guide should be the nvm recommendation I mentioned in this comment.

Best,
David

@zaaakher
Copy link
Contributor Author

zaaakher commented Dec 20, 2023

@davidjerleke Thank you for your time,

My apologies I thought it'd work even if my node version is higher than the one stated in .nvmrc.

Nevertheless I switched to node v18.13.0 and the same issue occurs. Here's a clip of the terminal during the whole process (Sorry about the static noise, just mute it 😅)

I'm on windows 10 btw.

I'm gonna add a note to CONTRIBUTING.md regarding the node version 👍

@davidjerleke
Copy link
Owner

@zaaakher thanks for the screen recording. That's very strange 😕. Because this project uses GitHub actions and they don't fail when setting it up. They run every time I push something to the repository or merge a PR. It seems like the GitHub actions use a different node version though:

gh-action

Which version of yarn are you using?

Best,
David

@zaaakher
Copy link
Contributor Author

My yarn version matches the one in the github actions. Although my npm version is a little old. Here are my versions 👇
image

I will try to match the versions in the github actions environment and I'll let you know

@davidjerleke davidjerleke added the documentation Improvements or additions to documentation label Dec 20, 2023
@zaaakher
Copy link
Contributor Author

image

I matched all versions to the github action environment and deleted all node_modules and ran yarn install again.

and I'm still getting the same error when I run yarn build


Btw everytime I run yarn build it seems to modify files in packages/embla-carousel-docs/src/components/Sandbox

image

And the change seems to be adding a space between each line for some reason

image

@davidjerleke
Copy link
Owner

@zaaakher can you try running yarn start from the root and see if you get a better error message on your terminal? Because the error messages don’t tell much right now.

@zaaakher
Copy link
Contributor Author

When I run yarn start after that broken yarn build I get gatspy to run locally on localhost:8000 with this error 👇

image

And that error is the same when I skip the yarn build command altogether.

I'm investigating left and right trying to pinpoint what's causing the error

@zaaakher
Copy link
Contributor Author

I couldn't get the docs to run locally no matter how hard I tried. and I spent too much time on it today 😅

I wish it ran smoothly so I can work better on #658 however I made that PR as a starting point and I'll try to improve on it more inshallah

@davidjerleke
Copy link
Owner

@zaaakher thank you for your time. I appreciate it. To be honest the error you get doesn't seem to be related to any Embla package because they don't call document.createElement anywhere in the code. It seems like Gatsby is failing. I have a unix based system which is different from windows. A wild guess is that you could try bumping the gatsby package one or two patch versions to see if that solves the problem.

Another thing you could try:

  • Create an empty folder called docs in the project root.
  • Run yarn build:docs.
  • Then try yarn build.

Thanks for your efforts.

@zaaakher
Copy link
Contributor Author

I did the following:

  • Deleted the existing /docs from the root
  • Created an empty docs folder in the root
  • Ran yarn install
  • Ran yarn build:docs
    And that last command deleted the docs folder during the build and then failed with three errors

image
image
image

@davidjerleke
Copy link
Owner

Thanks for testing @zaaakher. As I mentioned I have a Unix system which is different from Windows so I will try on a Windows machine when I get the chance and let you know how it goes.

@zaaakher
Copy link
Contributor Author

I just tested it on a Macbook Pro and everything worked extremely well without a single error 😄. So now i'm 100% sure it's a gatspy/window issue originating from embla-carousel-docs package

@zaaakher
Copy link
Contributor Author

zaaakher commented Dec 23, 2023

@davidjerleke I followed the instructions in this new docs section of CONTRIBUTING.md when I tested it in my unix-based environment and everything is working butter smooth. So I think this PR changes are complete, unless you have something you'd like to add to the documentation section of CONTRIBUTING.md.

If you'd like, I can create an issue addressing the gatspy/windows-10 build errors so that it can be dealt with separately without having to dig through the thread in this PR.

Thanks,
Zakher

@davidjerleke
Copy link
Owner

davidjerleke commented Dec 24, 2023

If you'd like, I can create an issue addressing the gatspy/windows-10 build errors so that it can be dealt with separately without having to dig through the thread in this PR.

@zaaakher I think that’s a great idea! I will be away for 3-4 days and won’t have access to my computer but when I’m back I will rebase and review this PR. There might be a thing or two I want to add before merging. Or maybe not. Regardless, thank you for your efforts!

It’s refreshing to see someone not just coming here pointing out missing things, but actually makes an effort to improve things for a change 👍🏻!

@zaaakher
Copy link
Contributor Author

@davidjerleke You're welcome brother 👍, I'm glad I could help and I appreciate the work you do and the wonderful library you made ✨

zaaakher and others added 4 commits December 26, 2023 21:12
@davidjerleke davidjerleke merged commit 5e09aa8 into davidjerleke:master Dec 26, 2023
@davidjerleke davidjerleke added the resolved This issue is resolved label Dec 26, 2023
@davidjerleke
Copy link
Owner

Thanks @zaaakher! This is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation resolved This issue is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants