Invalid examples in new fixtures files

[andymantell]

I am currently going through implementing the new fixtures released as part of 3.9.0 and have encountered an issue with some of the new hidden examples. Take the following from date input:

- name: with values
  hidden: true
  data:
    items:
      -
        id: day
      -
        id: month
      -
        id: year
        value: 2018

This example is missing name and label keys from each item which are marked as required: true in the specification of the component. This is just one instance of this kind of issue, I have encountered several others (Happy to compile a list of all the issues once we've bottomed out the discussion...)

I think I would expect all the examples in these files to be valid relative to the specification. It's causing me a slight issue in govuk-react-jsx whereby my components do not expect these values to be un-set. I had started to embark on making the components more resilient, but thought I would open this ticket before I go too far. I could make my components handle these invalid examples by guarding against these kinds of things, but is that the right thing to do? If people were omitting these attributes whilst actually using my library in the wild, it would be outputting incorrect markup - I actually want my library to throw errors when people do this, not silently swallow them.

Which leads me to think I don't want to guard against this, and that the base examples should be valid relative to the spec.

What do you think? Perhaps the build tool could even be validating examples against the spec to ensure these don't get through.

[andymantell]

Incidentally, label shouldn't actually be marked as required for date input items. If you omit label then name gets used instead.

[andymantell]

I retract the statement that there are other examples - this is the only one I've found so far (I was wrong about the others).

So I think it still stands that this date example is needs name attributes added, but the problem isn't as widespread as I first thought.

(I'll keep plugging through in case I find any other examples)

[andymantell]

There are other examples such as this on the select component:

  - name: attributes
    hidden: true
    data:
      attributes:
        data-attribute: my data value

Which is missing the items key which is marked as required in the select component specification. I guess this is acceptable in Nunjucks because this iterating over an undefined value like {% for item in params.items %} doesn't error whereas in JavaScript / JSX I would do items.map((option, index) which then says Cannot read property 'map' of undefined.

I think the way forward would be to validate the examples at some point during the build process. Happy to have a crack at this if people think it's a reasonable idea.

[andymantell]

Have done a spike into validating the examples and it's a bit of a can of worms. There are various fields which are marked as "required" but aren't actually treated as such (e.g. html/text or maxlength/maxwords on character-count). There are also some examples which will never validate such as:

- name: with falsey values
  hidden: true
  data:
    name: example-name
    items:
      - value: 1
        text: Option 1
      - false
      - null
      - ""
      - value: 2
        text: Option 2

So I don't think this is really a useful avenue, not without changing more things than I came here for. You can see the initial spike here: andymantell@2245d59#diff-ec4eb4c51af8d6a320c0398963c9620eR90

I will come at this again with a fresh head on Monday and try to more clearly articulate what I think the actual problem is. I think it basically amounts to fixing the ones that are causing errors in my JavaScript, but that I don't think I should be explicitly catering for as a "third party".

[vanitabarrett]

Hi @andymantell

Thanks for raising this issue and apologies that it’s preventing you using these new fixtures! Unfortunately the issues you’ve raised are intertwined with other things which will take a substantial amount of time to unpick and resolve, and we’re not able to tackle those at the moment. However, we are:

1. adding this issue to the backlog so we address the wider problem when we get more time
2. adding a new issue to address some of the yaml examples which are missing required fields, which we’ll pick up in the next few days

Hopefully that will help resolve the majority of issues for you!

[andymantell]

Thanks @vanitabarrett. My plan is to try and handle the irregularities but leave comments so I can remove the workarounds again in future, so I'm not blocked. I'll post a summary of my thoughts once I've worked through it all so you can judge whether to take action or not in each case.

[andymantell]

Hi @vanitabarrett - I've noticed a similar / related issue in that some of the fixtures contain lots of references to undefined. For example in the textarea examples you see things like:

"html": "<div class=\"govuk-form-group\">\n  \n\n  \n  \n  <div id=\"undefined-hint\" class=\"govuk-hint\">\n

I think that this is essentially the same issue and would be fixed once all examples contain the required attributes, but I just wanted to flag it for awareness. Don't know whether you want to track this in a separate ticket

[vanitabarrett]

Thanks @andymantell - actually working on this as we speak! 🙂

[andymantell]

ah, amazing :-) Thanks very much. Have a good weekend!

[colinrotherham]

Hi @andymantell, I spotted this issue again yesterday so good to add an update

Our fixtures are now all HTML validated with missing element checks, similarly for accessibility tests:

• Add HTML validation rules for missing elements #4417
• Switch Accessibility tests to @axe-core/puppeteer #3522

Which included a sweep to fix all the invalid or missing things:

e877ae3 Fix missing table heading text in component example
02616a1 Fix missing labels in component examples
a30bbe1 Fix missing component data fields
3ed9d1f Fix missing elements using id="content" on the preview container

Hopefully we've caught everything now (not forgetting description changes) but let us know if not

[andymantell]

I'm not currently anywhere near this any more, but I still greatly appreciate the hard work that's gone into it! Should make things better for everyone. Good job, cheers 👍🏻