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

Made CONTRIBUTING guide more (windows-)user friendly #2893

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

aGitForEveryone
Copy link
Contributor

@aGitForEveryone aGitForEveryone commented Jun 18, 2024

closes #2889

I had quite a few issues when trying to follow the contributing guide on my Windows system (also because I have very limited experience with working with the JavaScript ecosystem). After connecting to @ann-marie-ward and @BSd3v to get my system working, I wanted to share the gained knowledge to future (beginning) Dash contributors.

I added extra instructions and explanation to make the process a bit friendlier to new developers (and working on Windows systems like I do). Some other explanations where made a bit more verbose.

Contributor Checklist

  • No tests were run as this is only a change to CONTRIBUTING.md

optionals

  • I have added entry in the CHANGELOG.md (is this necessary?)

Added extra instructions and explanation to make the process a bit friendlier to Windows developers (and people very new to working with JavaScript). Some other explanations with made a bit more verbose, again to help beginner developers.
@T4rk1n
Copy link
Contributor

T4rk1n commented Jun 18, 2024

I will try on my windows computer this week and come back to review.

@aGitForEveryone
Copy link
Contributor Author

While we have the topic open. Can we also review the section on building the different parts after making changes? For the latest PR I made changes to the dash-renderer. To build the changes, I always ran renderer build after making any change I wanted to test out. However, it feels like this is too much for just adding a semi-colon. Would renderer bundles also work? The renderer section shows the different commands that are available, but I was missing some more explanation on what to do after I made a change and I wanted to test it.

Given that I don't fully understand the whole process (yet), I don't feel comfortable writing anything on it.

@BSd3v
Copy link
Contributor

BSd3v commented Jun 21, 2024

@aGitForEveryone

I don't know if I would want to go down trying to recommend different things if you alter different files in the renderer to recommend performing different run commands.

If your issue is just the build time for this, they are working on improving the speed at which these commands run. I mean, something like 5 minutes down to 5 seconds. So, what you are recommending doesn't make sense when Plotly is going to make it super fast to build. 😁

@aGitForEveryone
Copy link
Contributor Author

@BSd3v Good point, of course if things go faster, less other things become an issue. But I was actually facing another issue, which was that I actually didn't know what the correct command was to run to rebuild the renderer. The current guide just explains that there are 5 commands and what they do. But not really which of the 5 to run when something changed in the codebase. In the end I picked the build command because it seemed like the only possible choice.

Then, because the command takes a bit to run, it made me wonder whether I picked the correct one and if there wasn't a better choice. Which in my view points out that the guide could improve in that part.

Now, because I don't really know how everything works, I have no clue what is the correct process. So, I cannot update the guide in good confidence :O. Therefore, I was hoping that one of the experts could chime in here.

@BSd3v
Copy link
Contributor

BSd3v commented Jun 24, 2024

@aGitForEveryone,

After running the initial command, whichever codebase you are adjusting would be the one that needs to be run:

renderer - run renderer build
html - run html build
dcc - run dcc build
data_table - run data_table build

At least this is the way I understood it. 😄

@gvwilson
Copy link
Contributor

@T4rk1n did you get a chance to review this on your Windows machine? If so, can we merge? thanks - @gvwilson

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.

Contributing to Dash on Windows, contributing instructions unclear
4 participants