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 accordion macros about summary line change #2313

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

EoinShaughnessy
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy commented Aug 16, 2021

Addresses #2310.

What we've added

This PR adds content to the following parameters' Descriptions in the macros table for the accordion component:

  • summary.html
  • heading.html

Why we've added it

This new content tells users the summary line can only contain phrasing content. This is because using non-phrasing content will result in invalid HTML.

We've also advised users to:

  • keep content simple
  • reconsider their use of the accordion, if they'd planned to use non-phrasing content

@EoinShaughnessy EoinShaughnessy added documentation User requests new documentation or improvements to existing documentation 🕔 hours A well understood issue which we expect to take less than a day to resolve. accordion 2i labels Aug 16, 2021
@EoinShaughnessy EoinShaughnessy self-assigned this Aug 16, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 August 16, 2021 07:26 Inactive
@EoinShaughnessy EoinShaughnessy force-pushed the update-accordion-macros branch from 5c54cfc to 44579b0 Compare August 16, 2021 07:43
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 August 16, 2021 07:43 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 August 16, 2021 16:29 Inactive
@EoinShaughnessy
Copy link
Contributor Author

Content has passed internal review. Will now send it for pre-i.

@EoinShaughnessy
Copy link
Contributor Author

Content has passed pre-i. Will now send it for 2i.

@EoinShaughnessy EoinShaughnessy force-pushed the update-accordion-macros branch from 73ea4b4 to 169811f Compare August 17, 2021 08:17
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 August 17, 2021 08:18 Inactive
@EoinShaughnessy
Copy link
Contributor Author

@hannalaakso Content has passed 2i. Can you approve? I won't merge until we're ready to release the iteration. :)

@hannalaakso hannalaakso added this to the v4.0.0 milestone Aug 17, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 October 26, 2021 14:00 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 October 27, 2021 14:40 Inactive
@lfdebrux lfdebrux self-requested a review October 28, 2021 09:04
Copy link
Member

@lfdebrux lfdebrux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, just needs rebasing

@EoinShaughnessy EoinShaughnessy force-pushed the update-accordion-macros branch from 9b529e1 to af95abd Compare October 28, 2021 14:04
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 October 28, 2021 14:04 Inactive
src/govuk/components/accordion/accordion.yaml Outdated Show resolved Hide resolved
src/govuk/components/accordion/accordion.yaml Outdated Show resolved Hide resolved
src/govuk/components/accordion/accordion.yaml Outdated Show resolved Hide resolved
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just blocking this from being merged until we get a chance to chat about my comments above (although they aren't necessarily blocking).

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EoinShaughnessy Thanks for addressing my feedback! Your two proposed changes (below) just need to be committed and then this can be merged 👍

src/govuk/components/accordion/accordion.yaml Outdated Show resolved Hide resolved
src/govuk/components/accordion/accordion.yaml Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 November 11, 2021 08:09 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2313 November 11, 2021 08:10 Inactive
@EoinShaughnessy
Copy link
Contributor Author

@hannalaakso Thanks for reviewing! Do we need to wait for 4.0 release before merging?

@hannalaakso
Copy link
Member

@EoinShaughnessy We've started merging 4.0 things to main now so this can be merged in 👍 (for anyone's who's interested, we're putting a separate process in place if we ever find ourselves in a situation where we need to publish any more 3.x.x releases when 4.0 changes have already been merged into main).

@EoinShaughnessy EoinShaughnessy merged commit 4f11856 into main Nov 11, 2021
@EoinShaughnessy EoinShaughnessy deleted the update-accordion-macros branch November 11, 2021 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accordion documentation User requests new documentation or improvements to existing documentation 🕔 hours A well understood issue which we expect to take less than a day to resolve.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update docs to explain that accordion summary line can only contain phrasing content
6 participants