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

Include hidden status of the component examples in published fixtures #2031

Conversation

andymantell
Copy link
Contributor

@andymantell andymantell commented Nov 20, 2020

In order for downstream projects to use the component examples for rendering as well as for tests, it is useful to know which examples should be hidden from display since they were designed for tests only.

I have therefore modified the gulp script slightly to include that hidden field when packaging up the fixtures.

To give some context - I used to use the example yaml files directly in govuk-react-jsx to render the demonstration app. I am working on the 3.9.0 update which added the fixtures.json files and so am switching across to those, but these now contain additional hidden examples which were not there before, and don't make a lot of sense when rendered on screen since they are often so minimal. I would therefore like to hide these from display.

@andymantell
Copy link
Contributor Author

It occurs to me that the hidden flag could be renamed in the fixtures, to make it clearer what it means?

Or maybe just document it somewhere, wherever the fixtures are currently documented...

@vanitabarrett vanitabarrett added the awaiting triage Needs triaging by team label Nov 23, 2020
@kellylee-gds kellylee-gds removed the awaiting triage Needs triaging by team label Nov 23, 2020
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.

Thanks @andymantell. I've tested this locally by building the package and the hidden examples now correctly contain a hidden field in the fixtures.

Just need a second approval from someone in our team and then we can merge this.

I think that's a good idea to think about whether the hidden attribute could be more clearly worded. I think we could generally do with learning a bit more about how the fixtures are being used and if there are any improvements we could make so we should definitely look at the naming of things as part of that 👍

@andymantell
Copy link
Contributor Author

Thanks @hannalaakso

I guess once this is merged and released though it becomes harder to rename later. I understand the hidden flag because I've been loitering in this codebase for years, but to someone inspecting just the package that arrives in their node_modules folder I think that hidden might just be a bit cryptic. "I've got a ton of fixtures and some of them are "hidden". But why?!"

What about test-only: true (or something with similar sentiment but better words)

Alternatively - should the visible examples on the design system docs actually exhaustively demonstrate everything that the component is capable of? Perhaps there shouldn't be a concept of hidden examples?

@hannalaakso
Copy link
Member

I wonder if test-only or similar could also be interpreted as having multiple meanings? I think it's difficult to settle on a single phrase to explain all the complexities, perhaps especially when we don’t know that much about the users of the fixture files and what those users might need to do. The better option, as your earlier comment suggested, might actually be to add some documentation for the fixtures in the future where this could be explained. We could look at this separately at a later date.

Perhaps there shouldn't be a concept of hidden examples?

The component examples in the GOV.UK Frontend review app are useful to check against when we make changes that could cause a visual regression (ideally this type of testing would be handled by automated visual regression tests but for now we’re doing it manually). The examples that we’ve decided to hide typically cause no visual changes but might set a custom class etc. which is something we can run automated tests on. We therefore hide those examples to simplify our manual visual regression checks.

@hannalaakso
Copy link
Member

@andymantell Would you be okay to add this PR in the Changelog under Fixes?

@andymantell
Copy link
Contributor Author

Yup, on it now...

@andymantell andymantell force-pushed the output-hidden-state-of-fixtures-in-package branch from 3b9ce88 to 96c3c90 Compare November 30, 2020 10:04
@andymantell
Copy link
Contributor Author

@hannalaakso Done

@36degrees
Copy link
Contributor

I've pushed an extra commit to update the tests that we run on the built package to take the new hidden attribute into account.

This flagged that the hidden attribute was only being added when it's set in the YAML, which does make sense but to be consistent I've updated it to coerce it to a boolean so that it's present for every fixture.

@andymantell
Copy link
Contributor Author

Thanks @36degrees

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-output-hid-tknu6o November 30, 2020 13:42 Inactive
@36degrees 36degrees added this to the [NEXT] milestone Dec 1, 2020
36degrees added a commit to alphagov/govuk-frontend-docs that referenced this pull request Dec 15, 2020
alphagov/govuk-frontend#2031 adds a new `hidden` property to the objects in the fixtures list.

Update the fixtures included in the example JSON to include the new `hidden` property, and document it in the list of properties.
andymantell and others added 3 commits December 15, 2020 14:37
In order for downstream projects to use the component examples for
rendering as well as for tests, it is useful to know which examples
should be hidden from display since they were designed for tests only.
Remove the outer `expect.objectContaining` so that any additional top level keys in the fixture object cause the tests to fail.

Co-erce the hidden attribute to a boolean to ensure it’s always present, rather than only being present when it’s set in the YAML.
@36degrees 36degrees force-pushed the output-hidden-state-of-fixtures-in-package branch from 892f4d3 to 49ad231 Compare December 15, 2020 14:41
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-output-hid-tknu6o December 15, 2020 14:41 Inactive
@36degrees
Copy link
Contributor

I've rebased this so that the CHANGELOG entry is added in the right place – it was being appended to the list of fixes that went out in 3.10.1.

36degrees added a commit to alphagov/govuk-frontend-docs that referenced this pull request Dec 16, 2020
alphagov/govuk-frontend#2031 adds a new `hidden` property to the objects in the fixtures list.

Update the fixtures included in the example JSON to include the new `hidden` property, and document it in the list of properties.
@36degrees 36degrees changed the title Expose hidden status of component examples in packaged fixtures Include hidden status of the component examples in published fixtures Dec 16, 2020
@36degrees 36degrees merged commit ae201eb into alphagov:master Dec 16, 2020
@andymantell
Copy link
Contributor Author

Thanks 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

6 participants