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

Update descriptions for Nunjucks macro options + fixes #4450

Merged
merged 13 commits into from Nov 22, 2023

Conversation

colinrotherham
Copy link
Contributor

Adds new descriptions from alphagov/govuk-design-system#2929 (comment)

@colinrotherham colinrotherham added documentation User requests new documentation or improvements to existing documentation nunjucks labels Nov 10, 2023
Copy link

github-actions bot commented Nov 10, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-5.0.0-beta.1.min.css 114 KiB
dist/govuk-frontend-5.0.0-beta.1.min.js 37.93 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 77.58 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 72.89 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.53 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 38.11 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.38 KiB

Modules

File Size
all.mjs 69.22 KiB
components/accordion/accordion.mjs 21.47 KiB
components/button/button.mjs 4.5 KiB
components/character-count/character-count.mjs 21.05 KiB
components/checkboxes/checkboxes.mjs 5.63 KiB
components/error-summary/error-summary.mjs 5.89 KiB
components/exit-this-page/exit-this-page.mjs 15.89 KiB
components/header/header.mjs 3.71 KiB
components/notification-banner/notification-banner.mjs 4.34 KiB
components/radios/radios.mjs 4.63 KiB
components/skip-link/skip-link.mjs 3.61 KiB
components/tabs/tabs.mjs 9.47 KiB

View stats and visualisations on the review app


Action run for 8113c4f

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Nov 10, 2023

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 fieldset in alphagov/govuk-design-system#2929 (comment) (or nested hint and label for checkboxes and radios) but I've added them, can always keep tweaking

Copy link
Contributor

@CharlotteDowns CharlotteDowns left a 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

@colinrotherham colinrotherham marked this pull request as ready for review November 15, 2023 17:40
@colinrotherham colinrotherham added this to the v5.0 milestone Nov 15, 2023
@colinrotherham
Copy link
Contributor Author

@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.

@domoscargin domoscargin self-requested a review November 16, 2023 10:06
@colinrotherham colinrotherham changed the title Update descriptions for nested Nunjucks macro options Update descriptions for Nunjucks macro options + fixes Nov 16, 2023
@36degrees 36degrees self-requested a review November 17, 2023 10:19
Copy link
Member

@36degrees 36degrees left a 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.

@colinrotherham
Copy link
Contributor Author

@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:

  • description: Name attribute…
  • description: The name attribute…
  • description: The name attribute…

Possibly made worse now we're (mostly) code-wrapping attribute names, values etc

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4450 November 22, 2023 09:57 Inactive
@colinrotherham
Copy link
Contributor Author

Thanks @36degrees for all the comments, this is ready to review now

  • See commit 849cacb to remove “Additional options for…” from mandatory params
  • See commit 8113c4f with added “Can be used to…” to more optional nested components

Copy link
Member

@36degrees 36degrees left a 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! 👏🏻

@colinrotherham colinrotherham merged commit 7a237b5 into main Nov 22, 2023
44 checks passed
@colinrotherham colinrotherham deleted the component-data-descriptions branch November 22, 2023 10:44
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
owenatgov pushed a commit that referenced this pull request Jan 11, 2024
Except for Radios where the fieldset param appears as mandatory yet has `required: false`: #4450 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation User requests new documentation or improvements to existing documentation nunjucks
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants