plotly
Made CONTRIBUTING guide more (windows-)user friendly
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
- [x] 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?)
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.
@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. 😁
@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.
@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. 😄
@T4rk1n did you get a chance to review this on your Windows machine? If so, can we merge? thanks - @gvwilson
@marthacryan can you please have a look and let us know if it should be merged? overlaps in some places with your contribution PR. thanks - @gvwilson
@gvwilson @ann-marie-ward Thanks for pointing out the overlap, I wasn't aware of this PR!
@aGitForEveryone Here are some high-level notes on the structure here:
- I love your explanations of everything! They're super clear :) I do wonder if some of these are too fine-grained though. Where we can, I think we should link to other projects' documentation. For example, how to clone / fork a git repo. That's not really something that's specific to dash, so it could probably be a link to github documentation (this? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo). I also wonder if the section that's marked as "for javascript beginners" could be cut down a little. I'd love to keep this guide as concise as possible.
- +1 on the details tag!
- In #2951, I tried to move away from having a big code chunk that explains stuff in comments - this is going to create merge conflicts with your PR! Maybe I'll just wait and update that PR after this one goes in
Oh also another note: is the "Windows Configuration" section supposed to include "Set up JavaScript environment (for JavaScript beginners)" and "Working with PyCharm"? Because I think that both of those sub-sections pretty much apply to mac as well (I'd have to test locally but those are generally the commands I use for nvm and while I don't use PyCharm, I think it's pretty much the same on Mac?).
If you only meant to include that one paragraph under the "Windows Configuration" section, I'd suggest adding another header to separate the collapsible sections from the windows section. I also think it could make sense to change that section to just a paragraph that starts with (in bold) Notes on Windows Configuration.
@marthacryan Thanks for the feedback. I took another look at the contributing guide and reshuffled some things and even integrated your idea with splitting the code block. Please have a look at it and let me know what you think of the new changes.
Regarding the level of detail, I came into the process as a novice in JavaScript and everything around it. I had dabbled a bit with clientside callbacks, but I had no clue how to set up a working JavaScript environment and how to run separate code. So for me, a higher degree of handhelding would have been nice when setting up my system. I recon there are more people out there like me, as the only reason I came into contact with JavaScript was because Dash is using it.
The problem I had is that trying to first understand the JavaScript environment, trying to understand the Dash build process and then on top of that try to get it to work on a Windows machine was a bit too frustrating (which is why I quit my attempt last year to improve the loading component). Luckily there was Ann Marie and Bryan to help me out:)
So, my hope with the verboseness of the guide was to save others the same frustrations and get them focused faster on getting their local dev environment working. Then understanding JavaScript will come naturally.
In the new changes, I split up the JavaScript section in 2 parts and moved it out of the Windows section. I still used the details tag, so it is hidden by default, not cluttering the view for the users that do know JavaScript. In this way, I think it looks more concise.
Regarding the forking, I did refer to that same link as you mentioned. I just emphasized that that is good practice to follow. What I did do in that part is elaborate on the cloning part. The use of [email protected] to clone the repo confused me as I never use that. I always use the https:// url. So when I didn't see that option, it got me wondering whether [email protected] did something extra or special. Since it doesn't (as far as I know), I wanted to showcase that using the http:// url is also fine.