Remove example files govuk frontend #890
Conversation
These are only use in the 'all components' page and are not useful for our users.
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-890
|
I think these three commits are largely unrelated – could they be split up into separate PRs? |
|
If the context of the pull request is to remove the examples, then If I do that without updating the 'all components' page our tests will fail, so they can't be split up. If the context of the pull request is to use the yaml rather than hardcoding, we'd have lots of unused files which normally we would clean up in the same file if possible? Happy to split up if you think that makes it easier to understand. |
There was a problem hiding this comment.
For some reason I totally failed to recognise that the 'All components' page actually used the .njk files – I thought they weren't being used anywhere any more. Sloppy reviewing on my part – sorry Nick!
Now that I actually understand what this PR is doing, it looks like a really good cleanup and a great improvement to the 'All components' example page 👍 💯
I do wonder whether it would make sense to 'move' the all components page to '/components/all' and move it from the examples list to the components list? As it seems to have more in common with those pages than this one?
Also one relatively minor comment just to make sure we're picking the same example to 'represent' the components.
app/app.js
Outdated
|
|
||
| res.locals.componentData = components.map(componentName => { | ||
| let componentData = fileHelper.getComponentData(componentName) | ||
| let firstExample = componentData.examples[0] |
There was a problem hiding this comment.
This is assuming that the first example will be the 'default' example – to be consistent with what we do elsewhere can we re-use the logic from here to pull out whichever example has the name 'default'?
There was a problem hiding this comment.
I suppose I was not too concerned that this logic does get the default (even though it does by coincidence).
Let's go with your suggestion and have the same intent as elsewhere.
|
I like your suggestion about having an 'all' rather than in examples, will make that change. |
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-890
|
Need to change the tests since they pointed at the old examples route. |
| @@ -90,7 +90,10 @@ describe('frontend app', () => { | |||
| request.get(requestParamsHomepage, (err, res) => { | |||
| let $ = cheerio.load(res.body) | |||
| let componentsList = $('li a[href^="/components/"]').get() | |||
| expect(componentsList.length).toEqual(lib.allComponents.length) | |||
| // Since we have an 'all' component link that renders the default example of all | |||
| // components, there will always be one more expected link. | |||
|
Good stuff. |
This pull request removes the requirement for components to have a hardcoded example, and instead makes use of the first example as defined in the components 'yaml' config.
While these files are published to npm, as far as I know these are only used for internal documentation on the 'all components' page of the review applications.
It's also unlikely a user would use this functionality, since these files have hardcoded examples.
For this reason I'm determining this as not a breaking change, but a documentation related change.
https://trello.com/c/qM6PLrZj/264-spike-decide-whether-to-generate-example-html-files-from-the-examples-in-the-yaml-file-one-hour