Closed andymantell closed 11 months ago
Incidentally, label
shouldn't actually be marked as required for date input items. If you omit label
then name
gets used instead.
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)
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.
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: https://github.com/andymantell/govuk-frontend/commit/2245d599e79893aa07a272b12805682c11ee05e1#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".
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:
Hopefully that will help resolve the majority of issues for you!
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.
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
Thanks @andymantell - actually working on this as we speak! 🙂
ah, amazing :-) Thanks very much. Have a good weekend!
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:
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
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 👍🏻
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:
This example is missing
name
andlabel
keys from each item which are marked asrequired: 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.