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 guidance for v4.0.0 #1961

Merged
merged 15 commits into from
Dec 16, 2021

Conversation

calvin-lau-sig7
Copy link
Contributor

@calvin-lau-sig7 calvin-lau-sig7 commented Oct 20, 2021

This PR for the updated accordion guidance moves on from the working Google Doc.

@netlify
Copy link

netlify bot commented Oct 20, 2021

✔️ You can preview this change here:

🔨 Explore the source changes: 76f2274

🔍 Inspect the deploy log: https://app.netlify.com/sites/govuk-design-system-preview/deploys/61bb0851b120a1000846ed17

😎 Browse the preview: https://deploy-preview-1961--govuk-design-system-preview.netlify.app

@calvin-lau-sig7 calvin-lau-sig7 force-pushed the calvin-lau-sig7-accordion-guidance branch from 470011a to 2549f00 Compare October 20, 2021 15:05
@calvin-lau-sig7 calvin-lau-sig7 linked an issue Oct 20, 2021 that may be closed by this pull request
4 tasks
@calvin-lau-sig7 calvin-lau-sig7 self-assigned this Oct 20, 2021
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy left a comment

Choose a reason for hiding this comment

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

Awesome update, just added some comments. This is looking in wicked shape though. 👍🏻

@EoinShaughnessy
Copy link
Contributor

@calvin-lau-sig7 @hannalaakso and I were wondering whether the guidance should include any content about how the summary line should only contain 'phrasing content'. We're already adding info about phrasing content to the macros, but that's a pretty inconspicuous location for it...

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.

This is looking great, thanks @calvin-lau-sig7 👏

Do we want to include something to address the issues raised by a user in #1684 (comment) about teams nesting accordions?


Only add a summary line if it’s actually needed, as it's likely to make the button text too long.

If you’ve decided that you need the summary line, you must make it as short as possible.
Copy link
Member

@hannalaakso hannalaakso Dec 13, 2021

Choose a reason for hiding this comment

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

Should we try to revise the content in the example below it to use shorter summary lines? Just realised that it doesn't actually follow the spirit of our guidance.

Copy link
Contributor Author

@calvin-lau-sig7 calvin-lau-sig7 Dec 14, 2021

Choose a reason for hiding this comment

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

Probably something to look at later after we publish, perhaps with the service manual team on their live pages?
We've haven't seen anything live that fits and I'd find it difficult to make a thought-out example.

I'll suggest it in an issue so we can do it as a next step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as issue to look at after release:
#1999

Copy link
Member

@christopherthomasdesign christopherthomasdesign left a comment

Choose a reason for hiding this comment

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

Generally fantastic 💫 added a few detailed comments that seem to be echoed by Hanna

src/components/accordion/index.md.njk Outdated Show resolved Hide resolved
src/components/accordion/index.md.njk Show resolved Hide resolved
src/components/accordion/index.md.njk Outdated Show resolved Hide resolved
Copy link
Member

@christopherthomasdesign christopherthomasdesign left a comment

Choose a reason for hiding this comment

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

Read, re-read and re-read again. Looks great to me.

calvin-lau-sig7 and others added 5 commits December 16, 2021 09:34
Co-authored-by: EoinShaughnessy <72507742+EoinShaughnessy@users.noreply.github.com>
Co-authored-by: EoinShaughnessy <72507742+EoinShaughnessy@users.noreply.github.com>
Co-authored-by: Vanita Barrett <vanita.barrett@digital.cabinet-office.gov.uk>
@hannalaakso hannalaakso force-pushed the calvin-lau-sig7-accordion-guidance branch from 06140d6 to 76f2274 Compare December 16, 2021 09:35
@hannalaakso hannalaakso merged commit 2ff665e into main Dec 16, 2021
@hannalaakso hannalaakso deleted the calvin-lau-sig7-accordion-guidance branch December 16, 2021 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update accordion guidance
5 participants