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

Update JavaScript docs #808

Merged
merged 2 commits into from
Jun 21, 2018
Merged

Update JavaScript docs #808

merged 2 commits into from
Jun 21, 2018

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Jun 20, 2018 •
edited
Loading

This PR:

  • Updates the instructions for using JavaScript by grouping them under include or import
  • Document including the scripts, instead of importing them, as an option
  • Fixes out-of-date initialisation instructions.
  • Fixes some heading levels.
  • Remove a stray comment about FE being available to
    install as multiple packages
  • Updates Webpack example
  • Add some comments for extra guidance to src/all.js

Trello card

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 20, 2018 10:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 20, 2018 10:10 Inactive
alex-ju
alex-ju reviewed Jun 20, 2018
View reviewed changes
docs/installation/installing-with-npm.md Outdated

You can also initialise individual components using the `data-module` attribute set on the component. If you only have one instance of a component per page, you can follow the below example to initialise it:

```js
Copy link
Contributor

@alex-ju alex-ju Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The initialisation is dependent on the include/import method.

  • If we use import Radios from 'govuk-frontend/components/radios/radios' we assign the exported constructor to the Radios variable at import.
  • If we include the all.js script we need to refer the constructor from the GOVUKFrontend global variable var Radios = window.GOVUKFrontend.Radios

And then do the initialisation as shown below:

var $radio = document.querySelector('[data-module="radios"]')
if ($radio) {
  new Radios($radio).init()
}

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 20, 2018 16:17 Inactive
@hannalaakso hannalaakso changed the title Update JavaScript docs WIP: Update JavaScript docs Jun 20, 2018
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 21, 2018 08:46 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 21, 2018 08:55 Inactive
@hannalaakso hannalaakso changed the title WIP: Update JavaScript docs Update JavaScript docs Jun 21, 2018
NickColley
NickColley approved these changes Jun 21, 2018
View reviewed changes
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Looking really good. Some comments, that maybe we could give default recommendations rather than letting people decide. This'd make it easier for copy and paste folks. Happy to talk it through in person.

docs/installation/installing-with-npm.md Outdated
@@ -55,7 +52,7 @@ your Sass file:
@import "node_modules/govuk-frontend/components/button/button";
```

#### Optional: Resolving import paths
### Optional: Resolving SASS import paths
Copy link
Contributor

Choose a reason for hiding this comment

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

SASS -> SCSS

docs/installation/installing-with-npm.md Outdated

new Button().init()
#### Initialise all included components
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would make sense to be above the other, since I imagine most people will use this by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider merging this example with the place we tell people to include a script tag.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 21, 2018 14:12 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-808 June 21, 2018 14:14 Inactive
@hannalaakso
Update JavaScript docs
ee7f54a 
Document including the scripts, instead of importing them,
as an option.

Fix out-of-date initialisation instructions.

Fix some heading levels.

Also remove a stray comment about FE being available to
install as multiple packages.
@hannalaakso
Update changelog
821f2b6
@hannalaakso
Copy link
Member Author

Thanks for the IRL @NickColley. Could you check you're happy with the changes made before I merge this 🙂

@NickColley
Copy link
Contributor

Thanks Hanna, I think this'll help people a lot

@hannalaakso hannalaakso merged commit 6c2fe91 into master Jun 21, 2018
@hannalaakso hannalaakso deleted the update-js-docs branch June 21, 2018 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-ju alex-ju alex-ju left review comments

@NickColley NickColley NickColley approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@hannalaakso @NickColley @alex-ju @govuk-design-system-ci