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

Improve contribution guide & readme, add code of conduct #5068

Merged
merged 19 commits into from
Aug 14, 2020

Conversation

nicolaskruchten
Copy link
Member

Trying to make it a bit clearer how folks can contribute, so that we can link to this doc when saying "OK, the next step would be to propose some new attributes" or whatever :)

@nicolaskruchten
Copy link
Member Author

Once this is merged in, we can add some extra links in the Plotly.py contributing guide to provide more of a trail of how to get features added to graph_objects... now that we have actual doc pages to link to that explains that they're auto-generated from the JS schema.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@nicolaskruchten
Copy link
Member Author

Yeah, once folks are OK with the prose, I'll sprinkle in some links.

Copy link
Contributor

@nicholas-esterer nicholas-esterer left a comment

Choose a reason for hiding this comment

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

Seems good to me, just some little typos.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
nicolaskruchten and others added 2 commits August 12, 2020 12:30
Co-authored-by: NickE <64649603+nicholas-esterer@users.noreply.github.com>
@nicolaskruchten
Copy link
Member Author

Feedback incorporated, ready for more review from @antoinerg @jonmmease @emmanuelle :)

@jonmmease
Copy link
Contributor

I like it a lot! I don't see any changes needed.

@nicolaskruchten
Copy link
Member Author

One idea would be to identify specific issues or pull requests that exemplify this process ... any nominations for issues that led to good discussions/proposals that were then implemented as community PRs?

@nicolaskruchten
Copy link
Member Author

I expanded the top section a bit to provide a better "landing zone" for Python devs who land here wanting to contribute and needing orientation as to why they're in a JS project now :)

CONTRIBUTING.md Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Aug 12, 2020

@nicolaskruchten before merging, could you please fix the syntax?
Thanks.

@archmoj
Copy link
Contributor

archmoj commented Aug 12, 2020

Also I appreciate if you change in this PR the https://github.com/plotly/documentation/blob/source/Contributing.md link to https://github.com/plotly/graphing-library-docs/blob/master/README.md in the README.md file.

@nicolaskruchten
Copy link
Member Author

@archmoj fixed, and did some fixup work on the readme

README.md Outdated
|**MATLAB**| [plotly/matlab-api](https://github.com/plotly/matlab-api) | [plotly/matlab/getting-started](https://plotly.com/matlab/getting-started) |
|**node.js / Tonicdev / Jupyter notebook**| [plotly/plotly-notebook-js](https://github.com/plotly/plotly-notebook-js) | |

## Creators
## Maintainers
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maintain Creators in the title here.
What about Creators & maintainers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why the attachment to "creators" ? I don't love that that excludes community contributors... Maintainer at least is a fairly formal role

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a number of people in the list below (including Plotly contractors in the Hall of Fame) who were not in charge of "maintaining" plotly.js; however their contribution was remarkable.

In respect to community contributors I suggest we add the link below to the bottom of list: https://github.com/plotly/plotly.js/graphs/contributors.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about Maintainers for the top list and Notable Contributors for the bottom list? I don't love that Jon Mease isn't in the "Hall of Fame" for example... he did add a whole trace type :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely add Jon Mease to the top list as an active contributor not the bottom list.

README.md Outdated

| | GitHub | Twitter | Status |
|---|--------|---------|--------|
|**Mojtaba Samimi** | [@archmoj](https://github.com/archmoj) | [@solarchvision](https://twitter.com/solarchvision) | Active, Maintainer |
Copy link
Contributor

Choose a reason for hiding this comment

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

No. This is rather confusing. I am sure I should not be listed on top of this list.
Let's maintain the Hall of Fame.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screen Shot 2020-08-13 at 4 28 14 PM

@archmoj I think the contributor graphs make it pretty clear that you're the primary maintainer of this project since @etpinard's departure (and arguably on an equal footing to him for some time prior to that). So there should be no question that this placement is deserved. But if you would prefer to install me as a figurehead public face of the project I guess I won't protest.

Copy link
Member Author

Choose a reason for hiding this comment

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

that was my reasoning as well, but I'm happy to switch out the top two

Copy link
Member Author

Choose a reason for hiding this comment

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

Order switched.

## Enforcement

Instances of abusive, harassing, or otherwise unacceptable behavior may be
reported by contacting the project team at alex@plot.ly or accounts@plot.ly. All
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put another email of a specific person (for example Nicolas) and include full names (like "alex@plot.ly" (Alex Johnson)) so that people are easy to identify / google.

CONTRIBUTING.md Outdated
3. **Iteration** - The maintainers of the library or any other interested community member will then give feedback on the proposal, usually focused on consistency with the rest of the schema, and helping define a test plan to further elaborate potential edge cases.
4. **Approval** - After a number of iterations, the maintainers of the library will generally approve a proposal with an informal "this seems like something we would accept a pull request for" comment in the issue.
5. **Development** - A community member or maintainer creates a branch and makes the appropriate modifications to the code and tests and opens a pull request.
6. **Review** - The maintainers of the library will review the pull request, working with the original authors to ensure the code is ready for merging.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
6. **Review** - The maintainers of the library will review the pull request, working with the original authors to ensure the code is ready for merging.
6. **Review** - The maintainers of the library will review the pull request, working with the original authors to ensure the code is ready for merging. Iterations during review may take a bit of time, so be patient! In some cases it may also happen that the pull request cannot be merged for some reasons, but the process of starting by a discussion in an issue reduces the occurrence of this situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

or something like that :-). I wanted to make sure that people know that it's not a completely linear process, sometimes unexpected difficulties arise, or life happens, or...


Thanks for your interest in contributing to Plotly.js! We are actively looking for
diverse contributors, with diverse background and skills.

Copy link
Contributor

Choose a reason for hiding this comment

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

put here a few sentences explaining the organization of this document? It is quite long so it might be worth it. In particular some people will be looking specifically for out to set up a development environment.

Copy link
Contributor

@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

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

I like these additions a lot! I left just a couple of suggestions.

@archmoj
Copy link
Contributor

archmoj commented Aug 14, 2020

Could you please add regl to line 7 of README after stackgl?

Built on top of [d3.js](https://d3js.org/) and [stack.gl](http://stack.gl/),

@nicolaskruchten
Copy link
Member Author

nicolaskruchten commented Aug 14, 2020

Could you please add regl to line 7 of README after stackgl?

You can make these changes too if you like :)

I actually think we should remove this bit about d3... it's not particularly helpful or informative IMO.

@archmoj
Copy link
Contributor

archmoj commented Aug 14, 2020

Could you please add regl to line 7 of README after stackgl?

You can make these changes too if you like :)

I actually think we should remove this bit about d3... it's not particularly helpful or informative IMO.

I also think that line needs an update.

@archmoj
Copy link
Contributor

archmoj commented Aug 14, 2020

@nicolaskruchten thanks for the input.
You may consider updating the PR title and/or description to better reflect all the changes, if you like.
💃 💃 💃
if there are no other comments.

@nicolaskruchten nicolaskruchten changed the title Explanation of how changes get made Improve contribution guide & readme, add code of conduct Aug 14, 2020
@nicolaskruchten nicolaskruchten merged commit acf4e99 into master Aug 14, 2020
@archmoj archmoj deleted the nicolaskruchten-patch-1 branch August 14, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants