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 note for contributors on using hidden:true in yaml files #1922

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

m-green
Copy link
Contributor

@m-green m-green commented Aug 19, 2020

Part of #1830

As part of adding docs on testing with HTML fixtures, this adds a note in our contributing docs about using hidden: true in yaml files if you want your example to appear in the fixtures files but not the review app.

@m-green m-green added the documentation User requests new documentation or improvements to existing documentation label Aug 19, 2020
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1922 August 19, 2020 14:17 Inactive
@m-green m-green linked an issue Aug 19, 2020 that may be closed by this pull request
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1922 September 1, 2020 10:48 Inactive
@@ -37,7 +37,7 @@ You should add an example to the review app if the existing examples do not refl

If you've created a new component, create a new `src/govuk/<COMPONENT>/<COMPONENT>.yaml` file instead, where `<COMPONENT>` is the name of the component you've created.

Use `hidden: true` in an example if you want to include the example in our [test fixtures](http://frontend.design-system.service.gov.uk/testing-your-html/) but not the review app.
Use `hidden: true` in your example if you do not want to include the example in the review app. The example will still appear in our [test fixtures](http://frontend.design-system.service.gov.uk/testing-your-html/).
Copy link
Member

Choose a reason for hiding this comment

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

Would it be helpful to say that they would probably want the example to appear in the review app if there's a visual element to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that already covered in the first line of this section maybe? "You should add an example to the review app if the existing examples do not reflect the changes you've made."

Copy link
Member

Choose a reason for hiding this comment

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

We should actually update that line too as not all examples appear in the review app with this new change. Maybe something along the lines of:

You should add a new example if the existing examples do not reflect the changes you've made.

And then

If the example doesn't involve a visual change you can use hidden: true in your example to hide the example in the review app.

Copy link
Contributor Author

@m-green m-green Sep 1, 2020

Choose a reason for hiding this comment

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

Ah I see what you mean. I'm thinking this through... is it likely that an external contributor would add an example to the yaml file that would need hidden: true? Or is it mainly us who would be doing that?

I'm just wondering in retrospect whether to get into hidden: true here. Because including it might mean turning this section upside-down (because it's currently primarily focused on the review app).

Copy link
Member

@hannalaakso hannalaakso Sep 1, 2020

Choose a reason for hiding this comment

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

I think the rest of this section is still good in terms of the references to the review app.

I think a contributor could add a new example that needs to be hidden in the review app. If they aren't sure what we mean by 'visual change' they could ask us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Hanna - I've rejigged in 99267dc following your feedback and a chat with @36degrees. Hope it all makes sense now.

@@ -37,6 +37,8 @@ You should add an example to the review app if the existing examples do not refl

If you've created a new component, create a new `src/govuk/<COMPONENT>/<COMPONENT>.yaml` file instead, where `<COMPONENT>` is the name of the component you've created.

Use `hidden: true` in your example if you do not want to include the example in the review app. The example will still appear in our [test fixtures](http://frontend.design-system.service.gov.uk/testing-your-html/).
Copy link
Member

@hannalaakso hannalaakso Sep 1, 2020

Choose a reason for hiding this comment

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

I wonder if this new paragraph would make more sense being positioned after

  1. Add or update examples in the examples list.

as it's also about adding examples.

The 'If you've created a new component' paragraph feels like it starts a new topic as it's about adding a new file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes senses, thanks - will tweak.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1922 September 2, 2020 09:59 Inactive
@m-green
Copy link
Contributor Author

m-green commented Sep 2, 2020

Rejigged this in 99267dc following a chat with @36degrees - let me know if it's all ok!

Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Ready to merge once @hannalaakso has approved too

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Nice, a great tweak to the original content @m-green 📝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation User requests new documentation or improvements to existing documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draft documentation for using test fixtures
4 participants