Skip to content
This repository was archived by the owner on Feb 10, 2025. It is now read-only.

Updated contributing notes #27

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

Updated contributing notes #27

wants to merge 3 commits into from

Conversation

emmanuelle
Copy link
Contributor

No description provided.

@emmanuelle
Copy link
Contributor Author

@jvkersch would you mind taking a look at this quick update of the contributing notes? Having you onboard is a good opportunity to solve pain points which you might have encountered when getting started with the package.

Copy link
Contributor

@jvkersch jvkersch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks quite good! I had two further comments/comments, which may be because I'm not familiar with Plotly deployment conventions.

  1. I had to manually install webpack (npm install --save webpack) before the build:all command would work. Is this expected?
  2. The package.json file refers to the Python interpreter as venv/bin/python, which presumably assumes that there is a virtual environment called venv inside the package root directory. Would it make sense to assume that we're running within the virtual environment and refer to the Python interpreter simply as python? This would help with e.g. running within a Conda or EDM environment.

React and JavaScript, [this
tutorial](https://dash.plot.ly/react-for-python-developers) is a good
primer. In order to build the Javascript component: excecute at the root
of the package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also want to add a comment about installing the Python package? (e.g. pip install -e . or equivalent)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Since I'm terrified by Javascript but fluent with Python I assumed it would be the same for other contributors but it could be the other way around :-)

Co-Authored-By: Joris Vankerschaver <[email protected]>
@emmanuelle
Copy link
Contributor Author

Thank you for your comments. For webpack I have to investigate a bit; it is included in the dependencies in the package.json but maybe not at the right place...

@jvkersch
Copy link
Contributor

For webpack I have to investigate a bit; it is included in the dependencies in the package.json but maybe not at the right place...

I was looking at the contributing notes for dash-bio and they describe running npm install as the first step after cloning the repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants