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

Review feedback on test fixtures proposal #1885

Closed
vanitabarrett opened this issue Jul 28, 2020 · 3 comments
Closed

Review feedback on test fixtures proposal #1885

vanitabarrett opened this issue Jul 28, 2020 · 3 comments

Comments

@vanitabarrett
Copy link
Contributor

What

Review any feedback we receive on the test fixtures proposal.

Why

We have submitted the test fixtures proposal for feedback on the original issue. The deadline for comments is 5pm on 4th August, so we should review the comments after that.

@vanitabarrett vanitabarrett added this to Upcoming sprints in Design System Sprint Board via automation Jul 28, 2020
@vanitabarrett vanitabarrett moved this from Upcoming sprints to Sprint Backlog in Design System Sprint Board Jul 28, 2020
@kellylee-gds kellylee-gds moved this from Sprint Backlog to Upcoming sprints in Design System Sprint Board Jul 31, 2020
@kellylee-gds kellylee-gds moved this from Upcoming sprints to Sprint Backlog in Design System Sprint Board Aug 5, 2020
@vanitabarrett vanitabarrett moved this from Sprint Backlog to In progress in Design System Sprint Board Aug 5, 2020
@vanitabarrett vanitabarrett self-assigned this Aug 5, 2020
@vanitabarrett
Copy link
Contributor Author

The proposal generally got good feedback and most queries were handled by the existing approach. The following are the suggestions/questions that weren’t handled:

Making the fixtures.json an object rather than an array

This is recommended by the GDS API guidance (https://www.gov.uk/guidance/gds-api-technical-and-data-standards#use-json) and makes it easier for us to add additional properties in the future

This seems like a sensible suggestion to make our implementation more future-proof and reduce the possibility that we need to make any breaking changes.

I’m also going to suggest we add the component name into the fixtures JSON so users don’t need to analyse the file path to get the component name.

Handling caller

It looks like the fieldset component is the only component to support caller. It doesn’t currently have an alternative option that I can see (e.g: passing HTML via a html attribute).

It looks like the options are:

  1. We don’t support caller in the fixtures, but all other examples are tested
  2. We add a html param and example to the fieldset component

@36degrees @hannalaakso I've added 2 suggestions above for the queries we got on the proposal. Would be good to get your opinions (especially on the caller section)

@vanitabarrett vanitabarrett moved this from In progress to Needs review in Design System Sprint Board Aug 5, 2020
@36degrees
Copy link
Member

Making the fixtures.json an object rather than an array

This is recommended by the GDS API guidance (https://www.gov.uk/guidance/gds-api-technical-and-data-standards#use-json) and makes it easier for us to add additional properties in the future

This seems like a sensible suggestion to make our implementation more future-proof and reduce the possibility that we need to make any breaking changes.

I’m also going to suggest we add the component name into the fixtures JSON so users don’t need to analyse the file path to get the component name.

All of this sound sensible to me 👍🏻

Handling caller

It looks like the fieldset component is the only component to support caller. It doesn’t currently have an alternative option that I can see (e.g: passing HTML via a html attribute).

Huh. I am surprised, I thought we had a few components that supported it! We have an open issue to make it possible to call the details component (#941) and I think there's a few other components where it would make sense (inset text, warning text?)

It looks like the options are:

We don’t support caller in the fixtures, but all other examples are tested

I think we could still consider adding tests for the caller functionality, but equally I think this is something we could look at adding this in the future? I don't see it as a hard requirement for the initial implementation. However, suggest if we do so we make it a top-level property of the fixture object, rather than treating it as a special case in options:

{
  "examples": [ 
    {
      "name": "default",
      "options": {
        "legend": {
          "text": "What is your address?"
         }
      },
      "caller": "<p>Hello world</p>",
      "html": "<fieldset class=\"govuk-fieldset\">\n<legend class=\"govuk-fieldset__legend\">What is your address?</legend>\n<p>Hello World</p>\n</fieldset>"
    }
  ]
}

We add a html param and example to the fieldset component

I think we should do this regardless, as I think we should be treating call as an enhancement – there should always be a way of outputting the same HTML using the options alone. Where users are looking to mimic our API, this means this is possible in templating languages that don't have an equivalent to {% call %}.

@vanitabarrett
Copy link
Contributor Author

Added the following implementation details to the relevant cards:

@vanitabarrett vanitabarrett moved this from Needs review to Done in Design System Sprint Board Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants