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

Update readme in npm package #804

Merged
merged 2 commits into from
Jun 20, 2018
Merged

Update readme in npm package #804

merged 2 commits into from
Jun 20, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Jun 18, 2018

Currently the contents is out of date.
Proposed change includes parts of the main readme and parts of https://github.com/alphagov/govuk-frontend/blob/master/docs/installation/installing-with-npm.md with introduction bits and links to more details

Todo:

  • add correct DS URL
  • review JS initialisation

Adresses: #753

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 18, 2018 10:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 18, 2018 10:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 18, 2018 10:54 Inactive

### Build tool configuration
You can include Javascript for all components either by copying the `all.js` from `node_modules/govuk-frontend` into your application or referencing the file directly:
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are good but it makes me uneasy that they've not been made consistently in the installation section...

@TechPunk316
Copy link
Contributor

Hi all,

Reads well, I have a few comments/suggestions below:

  • Worth adding a link to NPM under the 'Requirements' subhead

  • Make the 3 'more details' links more descriptive:
    Suggest changing them to "More details on importing styles", "More details on importing Javascript and advanced options", "More details on importing assets"

  • Change "initialising it in your application to ensure that all users can properly use it"
    to "initialising Javascript in your application to ensure that all users can use it successfully"

  • Change "import assets" subhead to "importing assets" for consistency

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 19, 2018 15:42 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 19, 2018 15:51 Inactive
@hannalaakso
Copy link
Member

hannalaakso commented Jun 19, 2018

Thanks @TechPunk316 for the review! 👍

I've made the required changes and also reflected these in the installation docs.

Additionally, I've updated package README to include links to the two installation docs, this reflects the repo README.

I'll squash the commits once approved.

@36degrees 36degrees changed the title RFC: Update readme in npm package Update readme in npm package Jun 20, 2018
@36degrees 36degrees added this to the v1.0.0 milestone Jun 20, 2018

npm install --save govuk-frontend
Install the long-term support (LTS) version of [Node.js](https://nodejs.org/en/), which includes NPM. The minimum version of Node required is 8.6.0.
Copy link
Member

Choose a reason for hiding this comment

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

Is this true?

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 the minimum we require to contribute GOV.UK Frontend, not to consume it.

Copy link
Member

Choose a reason for hiding this comment

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

I've removed this section as it duplicates the npm installation instructions and is not true if you decide to install with option 2.

In the npm installation instructions, I've downgraded the version of node required to 4.2.0. which builds with nunjucks 3.0.0 and the nunjucks templates render.

Our contributing docs still say that Node 8.6.0 is required.

### 1. Install with npm (recommended)

We recommend [installing GOV.UK Frontend using node package manager
(npm)](docs/installation/installing-with-npm.md).
Copy link
Member

Choose a reason for hiding this comment

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

This link is broken – and when this is published to npm we can't reference another path relatively at all. Suggest making this a link including github.com (like we have for 'More details on importing styles')?

There's a few other links in this file that are also broken – I won't comment on them all.


To import add the below to your Sass file:

```CSS
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be scss for correct highlighting.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 14:47 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 14:50 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 14:52 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 15:00 Inactive
Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Few very minor comments, but this is looking ace 👌

npm install --save govuk-frontend
Once installed, you will be able to use the code from the examples in the
[GOV.UK Design System](https://www.gov.uk/design-system)
in your service.
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I don't think this needs to be wrapped onto a new line now that the URL is shorter.


You can include Javascript for all components either by copying the `all.js` from `node_modules/govuk-frontend` into your application or referencing the file directly:

```js
Copy link
Member

Choose a reason for hiding this comment

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

This is HTML.

```
Next you need to initialise the script by adding:

```js
Copy link
Member

Choose a reason for hiding this comment

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

As is this.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 15:11 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 15:13 Inactive
autoescape: true,
cache: false,
express: app
})

## Getting updates
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this text too?
To check whether you have the latest version of the button run:

Copy link
Contributor

Choose a reason for hiding this comment

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

Also To update the latest version run: may be clear as To update to the latest version run:

Copy link
Member

Choose a reason for hiding this comment

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

Good spot!

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-804 June 20, 2018 15:20 Inactive
Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

🎉

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.

None yet

7 participants