Skip to content

Conversation

@mkfreeman
Copy link
Contributor

This PR updates the instructions in the CONTRIBUTING.md file to more explicitly outline steps involved. The intention is to get people with less development experience empowered to make changes more easily. @zanarmstrong @ananya-roy I'd love to hear your thoughts on this -- if you have time to walk through the updated instructions, that would be a very helpful assessment!

Copy link

@ananya-roy ananya-roy left a comment

Choose a reason for hiding this comment

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

  • Under Development, it says to export the function to test/index.js. I think you meant test/plots/index.js

  • Are there guidelines for documenting changes (like a README or in some cases a published Observable notebook with a live example?) as part of submitting a PR?

@mkfreeman
Copy link
Contributor Author

Thanks @ananya-roy -- I've fixed the typo, and added a line about updating the README.md file (I'm not sure if there's any more detail we can provide as a general rule for updating the documentation).

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I want to reiterate some earlier feedback that was marked as resolved but was not addressed (at least, not explicitly).

The primary audience of CONTRIBUTING.md is the community (i.e. the public), not other Observable employees that can push directly to this repository. As such the recommended workflow for development, as @Fil pointed out, must be to fork this repository on GitHub rather than to clone this repository. In addition the {username}/{feature} branch naming convention need not apply; that is only needed because we, at Observable, by convention, share this repository.

@mkfreeman
Copy link
Contributor Author

Thanks for clarifying // resurfacing -- I've updated the Set Up instructions for the general public. I also switched to the default https code that GitHub has users copy (in case some doesn't have SSH keys set up). Happy to switch that back if desired.

@mkfreeman mkfreeman marked this pull request as ready for review January 27, 2022 18:16
CONTRIBUTING.md Outdated
```

## Submitting a pull request
Once you have completed your feature and confirmed that all test have passed using `yarn test`, you should push your changes up to GitHub and submit a [pull request](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request).
Copy link
Member

Choose a reason for hiding this comment

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

We mention pull requests up above (I suggest we move this GitHub link above), so maybe this section could instead be more specific about our expectations for what makes a good pull request, or a good feature? Like, please include a motivating example, or why you think this feature will be useful. Or, please consider filing an issue, or checking for existing issues, before submitting a pull request, in case someone else has already thought about this topic. Or perhaps to start, this section could instead be called ## Documentation, and delete this sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched the last section to Documentation and moved the Observable instructions there.

@mkfreeman mkfreeman requested a review from mbostock January 27, 2022 20:16
CONTRIBUTING.md Outdated
Comment on lines 30 to 33
If you’d like to create a new test plot as part of your development, you need to:

1. Create a new `.js` file in `test/plots`, e.g., `test/plots/new-example.js` (an easy start is duplicating an existing example).
2. Export the function from your `new-example.js` file in `test/plots/index.js`.
Copy link
Member

Choose a reason for hiding this comment

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

Isn’t this already covered below in the testing section, starting here?

To add a new snapshot test, create a new JavaScript file in the test/plots folder.

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 point -- as someone less familiar with this structure of development, I wouldn't think to look in the test section for information about how to create a new example (so I've left a sentence directing them there).

@mbostock
Copy link
Member

I still want to make a bunch of edits, but for the sake of expediency, I’ll just make them directly rather than going more rounds on this review. Thank you for the contribution. 🙏

@mkfreeman
Copy link
Contributor Author

Thanks so much, @mbostock -- I'm hopeful that this will make it easier for more people to contribute to Plot (saving time in the long run)

@mbostock
Copy link
Member

Apologies for the delay on this. I haven’t forgotten and will get to it soon.

@mbostock mbostock force-pushed the mkfreeman/contributing branch from 244af02 to 8abd360 Compare February 12, 2022 17:05
Co-authored-by: Philippe Rivière <[email protected]>
Co-authored-by: Mike Bostock <[email protected]>
@mbostock mbostock force-pushed the mkfreeman/contributing branch from 8abd360 to a6cdcb8 Compare February 12, 2022 17:05
@mbostock mbostock force-pushed the mkfreeman/contributing branch from fb11a8f to c49de4b Compare February 12, 2022 18:12
@mbostock mbostock merged commit 4a7a2b0 into main Feb 12, 2022
@mbostock mbostock deleted the mkfreeman/contributing branch February 12, 2022 18:14
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.

5 participants