davidjerleke / embla-carousel

A lightweight carousel library with fluid motion and great swipe precision.
https://www.embla-carousel.com
MIT License
5.39k stars 166 forks source link

Add docs section in CONTRIBUTING.md #657

Closed zaaakher closed 6 months ago

zaaakher commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

zaaakher commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 https://github.com/davidjerleke/embla-carousel/pull/658 however I made that PR as a starting point and I'll try to improve on it more inshallah

davidjerleke commented 6 months ago

@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:

Thanks for your efforts.

zaaakher commented 6 months ago

I did the following:

image image image

davidjerleke commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@davidjerleke You're welcome brother ๐Ÿ‘, I'm glad I could help and I appreciate the work you do and the wonderful library you made โœจ

davidjerleke commented 6 months ago

Thanks @zaaakher! This is now merged.