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.

Sign up for GitHub

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

Jump to bottom

Add prerequisites to README #33

Merged
merged 7 commits into from
Jan 25, 2018
Merged

Add prerequisites to README #33

merged 7 commits into from
Jan 25, 2018

Conversation

taveras
Copy link
Contributor

@taveras taveras commented Jan 17, 2018

Description

This change adds the environmental and dependency prerequisites for using the library. It also adds prerequisites which will be needed for development on the project.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore, documentation, cleanup

Related Issue

#23

Motivation and Context

We must communicate the needed detail around the environment where the library must be used.

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation (if required).
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@gautamarora gautamarora mentioned this pull request Jan 17, 2018
Open
28 tasks
@taveras taveras requested a review from pgoldrbx January 17, 2018 15:39
pgoldrbx
pgoldrbx reviewed Jan 17, 2018
View reviewed changes
README.md Outdated
@@ -4,6 +4,10 @@

Converts an XML document into a React tree.

## Prerequisites

- React 0.13.x or greater
Copy link
Contributor

@pgoldrbx pgoldrbx Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

let's add node minimum version also? oh, it's below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rollup does output three bundles: a CommonJS module, an ES2015 module, and a bundle for direct use in the browser. as we don't require node, we may end up needing something similar to the following:

### Prerequisites

##### Node.js
  - node.js version 0.10.x or greater
  - npm version 1.x or greater

##### Browsers
  - Chrome version $X or greater
  - Safari version $Y or greater
  - Firefox version $Z` or greater

overall, xml-to-react and xmldom are not using any special API methods within either runtime environment, so our needed browser version numbers would mirror React's requirements

is it valuable to separate the prereqs. by runtime environments, and highlight these browser versions? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm. i'm not entirely sure, to be honest. feels like a real chore

pgoldrbx
pgoldrbx reviewed Jan 17, 2018
View reviewed changes
README.md Outdated
- Ensure the filename ends with `.test.js`
- Run the linter and unit test suite with the following command:
```sh
$ npm run test
Copy link
Contributor

Choose a reason for hiding this comment

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

test is known to npm. so this can just be npm test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh! 😆

@taveras taveras force-pushed the update/prerequisites branch 2 times, most recently from a9e1238 to d437912 Compare January 18, 2018 04:48
@taveras
Copy link
Contributor Author

taveras commented Jan 18, 2018

@pgoldrbx ended up adding the Node.js and npm requirements.
i'm okay with explaining how to use the browser bundle in the forthcoming examples

how does this look?

@gautamarora
Copy link
Contributor

It also adds prerequisites which will be needed for development on the project.

I've seen docs like these go into the CONTRIBUTING.md as they are more relevant to someone wanting to contribute.

gautamarora
gautamarora reviewed Jan 18, 2018
View reviewed changes
README.md Outdated
@@ -103,6 +110,20 @@ The `XMLToReact` class is instantiated with a map of converters.
- `xml` `{string}` - xml node or document
- `data` `{Object}` - (optional) any data to be passed to all converters

## Development
Copy link
Contributor

Choose a reason for hiding this comment

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

as per note in the PR, i think this should live in CONTRIBUTING.md

gautamarora
gautamarora reviewed Jan 18, 2018
View reviewed changes
README.md
@@ -8,6 +8,13 @@ _Proudly built by:_

<a href="https://technology.condenast.com"><img src="https://user-images.githubusercontent.com/1215971/35070721-3f136cdc-fbac-11e7-81b4-e3aa5cc70a17.png" title="Conde Nast Technology" width=350/></a>

## Prerequisites
Copy link
Contributor

Choose a reason for hiding this comment

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

are there other similar projects that list pre-reqs?

Copy link
Contributor

Choose a reason for hiding this comment

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

funny that i am asking and its in the checklist 😆 but I am not finding others that do the same, so reconsidering this as an item in the README. open to thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

angular cli has one, though it is a tool instead of a library.

we should highlight the required version of React, but the other details do not seem as important 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i like how they list it out in 1 sentence. Somehow easier to grok than a list. Also, dont want our readme to be excessively long.

@taveras
Copy link
Contributor Author

taveras commented Jan 22, 2018

@gautamarora moved the development prerequisites and re-worded the README change to be a sentence. how do we feel about this?

pgoldrbx
pgoldrbx reviewed Jan 22, 2018
View reviewed changes
CONTRIBUTING.md Outdated
@@ -18,6 +18,21 @@ Open a [new issue](https://github.com/CondeNast/xml-to-react/issues/new). Follow

Open a new pull request. Follow the provided template as a guideline.

### What are the local environment requirements for development?

You must use Node.js version 8.9.x and npm version 5.x for development.
Copy link
Contributor

@pgoldrbx pgoldrbx Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

is 8.9 a hard dep here or is any 8.x ok? There will naturally be future release of node, so we'll have to maintain this. Is it worth adding to an .nvmrc as well? (duh)

@gautamarora
Copy link
Contributor

@taveras i created a dedicated "Development" section in the CONTRIBUTING.md, thoughts?

@taveras
Copy link
Contributor Author

taveras commented Jan 23, 2018

this lgtm! i changed the "Attribution" header to be on the same level as the "Development".

@pgoldrbx @gautamarora looks good to merge?

@taveras taveras merged commit 119a2ff into master Jan 25, 2018
@taveras taveras deleted the update/prerequisites branch January 25, 2018 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgoldrbx pgoldrbx pgoldrbx left review comments

@gautamarora gautamarora gautamarora left review comments

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@taveras @gautamarora @pgoldrbx