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 docs on testing with HTML fixtures #74

Merged
merged 2 commits into from
Sep 14, 2020
Merged

Conversation

m-green
Copy link
Contributor

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

Part of alphagov/govuk-frontend#1830

This adds a new docs page about testing your HTML matches GOV.UK Frontend.

Before merging we need to:

  • get the content pre-i'd and 2i'd
  • update the code example with the final code

@m-green m-green self-assigned this Aug 19, 2020

For each example, pass `options` into your own macro and check the generated HTML matches `html`.

Your HTML may not match exactly. For example, your framework may add extra whitespace or attributes to your HTML.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@36degrees @vanitabarrett I had a long look at the final paragraph and realised I might be trying to over-explain things. In retrospect, it feels like enough to warn users that the HTML may not match, without going into what to do about it - as this would be fairly self-explanatory? Let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, to me this implies that we might be doing something our end so that if their HTML doesn't match correctly it's ok and their tests will somehow still pass - but that might just be me! 🤷

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 the word 'may' makes it sound permissive? ('It's OK if the HTML doesn't match exactly')

Do we just need to be clear that they will have to do something…? What about something like…

If your HTML does not match exactly – for example if your framework adds extra whitespace or attributes to your HTML – you'll need to write your tests to allow for known differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've had another go in ee2717b - I've mainly used Ollie's suggestion but with slight style tweak. Let me know what you think!

@netlify
Copy link

netlify bot commented Aug 19, 2020

Deploy preview for govuk-frontend-docs-preview ready!

Built with commit eedad4e

https://deploy-preview-74--govuk-frontend-docs-preview.netlify.app

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.

Looks good 👍 Just left one comment.

We could also consider adding something the package README to document the HTML fixtures since it already explains what the macro options file is for. I can create another issue for that if it's something we want to do?

source/testing-your-html/index.html.md.erb Show resolved Hide resolved
Copy link
Contributor

@36degrees 36degrees 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 looking good to me @m-green, bar not being 100% sure about the callout.


If you [installed GOV.UK Frontend with Node.js package manager (npm)](/installing-with-npm), you can find the `fixtures.json` file for each component in the `node_modules/govuk-frontend/govuk/components/COMPONENT-NAME/` folder, where `COMPONENT-NAME` is the name of the component.

<%= warning_text('You should not test your HTML using the YAML files in the <code>src/</code> folder.') %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not 100% sure that this is terribly important to mention – or at least that it's important enough to warrant being in a callout. If people do it nothing bad's going to happen, it's just that we might change those files without giving them any warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No probs - happy to remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in eaeabe9

@vanitabarrett vanitabarrett added this to the v3.9.0 milestone Sep 2, 2020
@hannalaakso hannalaakso merged commit ecea395 into master Sep 14, 2020
@hannalaakso hannalaakso deleted the add-html-test-fixtures branch September 14, 2020 14:40
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.

4 participants