-
Notifications
You must be signed in to change notification settings - Fork 338
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
Component test files use examples #1924
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really great so far, I've now reviewed the following:
- warning text
- textarea
- tag
- tabs
- table
- summary list
- skip link
- select
- radios
src/govuk/components/radios/__snapshots__/template.test.js.snap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Reviewed
- phase banner
- panel
- label
- inset text
- input
916e650
to
4614684
Compare
@@ -147,3 +147,37 @@ examples: | |||
text: Section B | |||
content: | |||
html: <a class="govuk-link" href="#">Link B</a> | |||
|
|||
# Hidden examples are not shown in the review app, but are used for tests and HTML fixtures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I've been working on adding the new prefix/suffix examples it strikes me that having the hidden examples at the end of the file makes it harder to scan the file to understand what examples already exist and having to move between the "visible" and "hidden" examples for the same thing which is kind of annoying.
This might just be me - but would be interesting to hear if either of you have any thoughts on how we organise the new examples in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to separate them out as when they were all together I found it difficult to quickly see which examples were shown in the review app and which weren't. But perhaps you'd only really need to do that when working on the fixtures, and most people would benefit from having all examples together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make sense to me to group the examples for the same thing together 👍 If they're being added to the end of the file with no groupings it might become difficult to find anything there and if you're a contributor who's looking to potentially add a new example it would be helpful to see what already exists for that variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do get that you'd need to move things in several files to do that @vanitabarrett. I don't know if @36degrees has any thoughts before you do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect no matter our intentions they're going to get muddled up over time anyway 😬 Which might be an argument for not keeping them together, unless we think we can enforce that separation going forward? Do you think there's a logical way to order / group the examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have said the order in which they first appear in the test file, this seems to roughly follow the order of
default
with text
with html
with classes
with attributes
which would hopefully make the yaml easier to follow.
But I get your point that since we won't be programmatically enforcing it why even try to stick to a particular order.
4614684
to
c7336c2
Compare
c7336c2
to
410b844
Compare
410b844
to
1e657a5
Compare
1e657a5
to
d2234cd
Compare
d2234cd
to
0bd7652
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy that any issues I flagged previously on the components that I reviewed have been addressed – and that this can be merged once @hannalaakso has approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me too 👍
Resolves #1894
What
caller
Why
Prep work for #1895
Note: I've left any examples that were previously visible in the review app as-is for now as I'm not sure what the reasoning was behind adding those particular examples as something to check. It's possible some of those examples could also be hidden with the new flag.