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

Add component options (args) to YAML, add as JSON to package #998

Merged
merged 4 commits into from
Sep 21, 2018

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Sep 13, 2018

We want to expose the component options (arguments) to GOV.UK Design System. To do this, they need to be generated for inclusion in package.

This PR:

  • Adds all component options in the component YAML as params.
  • Introduces isComponent option (so that the options for that component can be linked to in the Design System).
  • Copies the component options as JSON files to package when gulp build:package --destination 'package' is run.
  • Amends the tests for checking contents of package to allow filenames with wildcards in additionalFilesNotInSrc
  • Adds a test to check all package/components have a macro-options.json that contains valid JSON with the expected attributes name, type, required, description

Created this as one PR so that the generated JSON files can be tested against alphagov/govuk-design-system#518

More on component options:

  • We now refer to these as “options” in client facing docs as calling them “arguments” is technically incorrect and “parameters” was felt to be less clear than “options”.
  • Regarding the "or" relationship between text and html attributes, we are continuing to explain this in the description of the attributes, ie. "If html is set, this is not required." There didn't seem to be a sensible way of trying to show this relationship in a simple table so using just text appeared to be the most accessible method.
  • This PR retains the convention of referring to them as “params” in component macro and template files - we might want to roll out renaming “params” to “options” in all component macros and templates as a separate PR that can be quickly merged in without causing too many conflicts with other PRs.

To test, run npm run build:package

https://trello.com/c/QN0imhXt/722-define-arguments-for-components-in-the-component-yaml-file
https://trello.com/c/00iDbQOU/1323-allow-argument-yamls-to-be-transformed-into-json-and-copied-to-package

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 13, 2018 16:04 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 13, 2018 16:27 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 15, 2018 17:48 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 16, 2018 07:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 17, 2018 11:33 Inactive

try {
let json = yaml.safeLoad(fs.readFileSync(componentPath), {encoding: 'utf8', json: true})
let paramsJson = {'params': json.params} // We only want the 'params' data from component yaml
Copy link

@kr8n3r kr8n3r Sep 17, 2018

Choose a reason for hiding this comment

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

should we stop the task if the yaml file doesn't contain the params data ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in a try block so it will throw up an error if that happens and the tests will also fail. That's probably okay? This is in the middle of the copy-files task so if we exit here, there might already be files copied over to package and others deleted.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 17, 2018 16:49 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 18, 2018 12:30 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 18, 2018 13:09 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 18, 2018 13:15 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 18, 2018 13:19 Inactive
@hannalaakso hannalaakso changed the title WIP: Add component options (args) to YAML, add as JSON to package Add component options (args) to YAML, add as JSON to package Sep 18, 2018
- name: caller
type: nunjucks-block
required: false
description: Not strictly a parameter but <a href="https://mozilla.github.io/nunjucks/templating.html#call">Nunjucks code convention</a>. Using a `call` block enables you to call a macro with all the text inside the tag. This is helpful if you want to pass a lot of content into a macro. To use it, you will need to wrap the entire fielset component in a `call` block. See <a href="https://github.com/alphagov/govuk-frontend/blob/master/src/components/checkboxes/template.njk#L86">checkboxes component</a> for an example.
Copy link
Contributor

@joelanman joelanman Sep 18, 2018

Choose a reason for hiding this comment

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

We're using html here for the links. We're also using ` backticks for option names, which is a markdown convention. We don't have to do anything with the backticks, but it's probably best to run them through a renderer to get <code> blocks. In which case maybe we should just use markdown for the links too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good shout. I was avoiding markdown since I only needed to use HTML in this one place for the component options and we discovered we'll need to do some extra processing as part of the build in the Design System to render markdown from here. But since we've got the code blocks too then making it all markdown makes sense 🙂

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 18, 2018 17:26 Inactive
let paramsJson = json.params // We only want the 'params' data from component yaml
file.contents = Buffer.from(JSON.stringify(paramsJson, null, 4))
} catch (e) {
console.log('ENOENT: no such file or directory: ', file)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be console.error. Also the try section has 3 lines in it, so we don't know for sure that this is the error. I'd recommend trying to reduce the try to just opening the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've split up the error messages more, have a look.

let componentPath = path.join(configPaths.components, componentName, `${componentName}.yaml`)

try {
let json = yaml.safeLoad(fs.readFileSync(componentPath), {encoding: 'utf8', json: true})
Copy link
Contributor

Choose a reason for hiding this comment

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

the readFileSync could be pulled out into a line above to make it more readable and have fewer operations happening in a single line

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed!

@joelanman
Copy link
Contributor

Do we still have a frontend test app? Maybe we can read the macro options json and write to a page in that, so we can see it works?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 19, 2018 15:32 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 19, 2018 15:38 Inactive
@hannalaakso
Copy link
Member Author

hannalaakso commented Sep 19, 2018

@joelanman Thanks a lot for the review 🙌

We test if the file contains JSON and if the JSON contains attributes (name, type, required, description) that we would expect it to have. Do you there are specific advantages to testing it in the test app or anything in particular we should look out for?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 20, 2018 15:49 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 20, 2018 16:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 20, 2018 17:33 Inactive
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.

All the components have been reviewed by the team and given a 👍 so I think this is good to go.

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.

We need to add a CHANGELOG entry

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 21, 2018 14:17 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 21, 2018 14:18 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 21, 2018 14:20 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 21, 2018 14:20 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-998 September 21, 2018 14:21 Inactive
hannalaakso and others added 4 commits September 21, 2018 15:45
Call files `macro-options.json` as only that data is copied over to package.
`additionalFilesInSrc` can now accept wildcards in filenames,
allowing us to add new files to `package/components`.

The test checks if all components have a `macro-options.json` and
whether it contains JSON with expected attributes "name", "type", "required"
and "description".
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.

This is a ton of work, thanks a lot Hanna 👍

@hannalaakso hannalaakso merged commit 3150e00 into master Sep 21, 2018
@hannalaakso hannalaakso deleted the add-args-as-yaml branch September 21, 2018 15:17
@kr8n3r kr8n3r mentioned this pull request Sep 26, 2018
hannalaakso added a commit that referenced this pull request Oct 8, 2018
As per changes made in #998
and alphagov/govuk-design-system#518 update
contributor docs on component options
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.

6 participants