-
Notifications
You must be signed in to change notification settings - Fork 335
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
Update descriptions for Nunjucks macro options + fixes #4450
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 8113c4f |
315b861
to
b34f754
Compare
Mind taking a look at some suggestions of mine please @CharlotteDowns? Mainly to swap "Can be used to add a…" with "Additional options for…" when defaults are already provided My fault for not including |
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 to me
9244289
to
ee0f0a1
Compare
@36degrees Shall we add in some of your suggestions from #3919 before merging this? One of them was moving the "this is not required" text to the end: - description: If `text` is set, this is not required. The heading HTML content of each section. The header is inside the HTML `<button>` element, so you can only add [phrasing content](https://html.spec.whatwg.org/#phrasing-content) to it. If `html` is provided, the `text` option will be ignored.
+ description: The heading HTML content of each section. The header is inside the HTML `<button>` element, so you can only add [phrasing content](https://html.spec.whatwg.org/#phrasing-content) to it. If `html` is provided, the `text` option will be ignored. If `text` is provided, this is not required. |
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.
This generally looks like a great improvement, but there are a few places where there are a few inaccuracies – a few of them are existing issues.
There's also a few places where we've aligned the description to the name of the option (like talking about 'items' more) but in doing so made it less useful in terms of explaining what items
actually means.
packages/govuk-frontend/src/govuk/components/accordion/accordion.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/character-count/character-count.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/cookie-banner/cookie-banner.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/date-input/date-input.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/date-input/date-input.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/summary-list/summary-list.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/summary-list/summary-list.yaml
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/task-list/task-list.yaml
Outdated
Show resolved
Hide resolved
@36degrees I've pushed up all those great suggestions Not urgent, but we jump between "The thing" versus "Thing" which we could tackle at some point:
Possibly made worse now we're (mostly) code-wrapping attribute names, values etc |
c0cee94
to
027465f
Compare
The status can be text without a tag (optional)
The label is rendered using `params.label` and is required
Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Except for Radios where the fieldset param appears as mandatory yet has `required: false`: #4450 (comment)
027465f
to
8113c4f
Compare
Thanks @36degrees for all the comments, this is ready to review now |
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.
Some really nice improvements here, thanks @colinrotherham and @CharlotteDowns! 👏🏻
Except for Radios where the fieldset param appears as mandatory yet has `required: false`: #4450 (comment)
Adds new descriptions from alphagov/govuk-design-system#2929 (comment)