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

RFC: Code of Conduct and updated Issue/PR templates #1052

Merged
merged 7 commits into from
Jun 3, 2018

Conversation

aem
Copy link
Contributor

@aem aem commented Mar 15, 2018

Here's a proposal for a Code of Conduct (with pieces of both the WeAllJS and Facebook templates), and PR/Issue templates (a combination of the existing content and the React templates).

Any feedback is welcome!

cc/ @bvaughn @diogofcunha

@codecov-io
Copy link

codecov-io commented Mar 15, 2018

Codecov Report

Merging #1052 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1052   +/-   ##
=======================================
  Coverage   90.33%   90.33%           
=======================================
  Files          59       59           
  Lines        1728     1728           
=======================================
  Hits         1561     1561           
  Misses        167      167

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb68696...fef0c7f. Read the comment docs.

@aem aem added the discussion label Mar 15, 2018
Copy link
Contributor

@diogofcunha diogofcunha left a comment

Choose a reason for hiding this comment

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

Nice one, just added some thoughts for discussion


**What is the expected behavior?**

**Which versions of react-virtualized, and which browser / OS are affected by this issue? Did this work in previous versions of React?**
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/bvaughn/react-virtualized/blob/master/package.json#L162 probably also want some info about react version as we support multiple?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "previous versions of r-v?"


**What is the current behavior?**

**If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:**
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the react code boxes, we should probably replace this with the ones we already had for this project?

https://codesandbox.io/s/03qpzq1p9p?module=%2FExample.js
https://plnkr.co/edit/6syKo8cx3RfoO96hXFT1


-->

**What is the current behavior?**
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 probably highlight the section splits with h3 or h2 to make a noticeable difference with the descriptions, thoughts?

* Plnkr: https://plnkr.co/edit/6syKo8cx3RfoO96hXFT1

# Requesting a Feature?
<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we just delete instead of comment?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure I understand the use of the comment tag

Even though these are markdown files, when you open a new issue- they're going to appear in plaintext. (See here for an example)

So the comment tag won't actually work, nor will any of the fancier formatting :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bvaughn that's intentional. i think those are things we want people to think about, but text that we don't want showing up in the final issue that's opened, so if it's there as a comment we don't have to depend on people to delete the text. should keep the same intent of the original content without cluttering up issue descriptions.

Copy link
Owner

Choose a reason for hiding this comment

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

I see. I would expect people to delete the contents of the issue template (and replace with their own issue description, etc.)

It doesn't make much sense to leave the template content in the issue, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok! happy to uncomment and just leave it there as guiding text

@aem
Copy link
Contributor Author

aem commented Mar 21, 2018

everyone's had some time to think this one over. are we feeling good about where these are at?


### What is the current behavior?

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React and react-virtualized. Paste the link to your JSFiddle (https://codesandbox.io/s/03qpzq1p9p?module=%2FExample.js) or Plnkr (https://plnkr.co/edit/6syKo8cx3RfoO96hXFT1) example below:
Copy link
Contributor

Choose a reason for hiding this comment

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

jsfiddle, plnkr or codesandbox?

Can we keep it to codesandbox? its easiest for maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 some of this was just a merge of the existing templates and the React templates, i'll pare it down to just codesandbox

| Browser | |
| OS | |
| React | |
| react-virtualized | |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add react-dom

@wuweiweiwu
Copy link
Contributor

Looks awesome!

@aem aem merged commit dcef5bc into master Jun 3, 2018
@aem aem deleted the aem/rfc-coc-templates branch June 3, 2018 01:32
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.

5 participants