Add documentation for 'id' to accordion component #3789
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/components/accordion/index.md
Outdated
| @@ -65,6 +65,8 @@ There are 2 ways to use the accordion component. You can use HTML or, if you’r | |||
|
|
|||
| The accordion component uses JavaScript. When JavaScript is not available, users will see all the content displayed with the section labels as headings. | |||
|
|
|||
| The 'id' attribute on the the parent div of the accordion is a required attribute. It must be unique across the domain of your service to persist the expanded state of the accordion (the state individual instances of the component persists across page loads using [session storage](https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage)). | |||
There was a problem hiding this comment.
Is it possible to make this into the active voice without changing its meaning? For example - You'll require the 'id' attribute from the parent div of the accordion.
Also, I'm unclear what you mean in the last sentence in brackets.
Suggested change
| The 'id' attribute on the the parent div of the accordion is a required attribute. It must be unique across the domain of your service to persist the expanded state of the accordion (the state individual instances of the component persists across page loads using [session storage](https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage)). | |
| The 'id' attribute on the the parent div of the accordion is a required attribute. It must be unique across the service's domain to maintain the persistent expanded state of the accordion (the state individual instances of the component persists across page loads using [session storage](https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage)). |
patrickpatrickpatrick
force-pushed
the
add-documentation-for-accordion
branch
from
467b689
to
e7ac344
Compare
src/components/accordion/index.md
Outdated
| @@ -65,6 +65,8 @@ There are 2 ways to use the accordion component. You can use HTML or, if you’r | |||
|
|
|||
| The accordion component uses JavaScript. When JavaScript is not available, users will see all the content displayed with the section labels as headings. | |||
|
|
|||
| If you are using HTML, you must add an 'id' attribute to the parent div of the accordion. It must be unique across the service's domain to maintain the persistent expanded state of the accordion | |||
There was a problem hiding this comment.
This is great. To be a nuisance, could you add a full stop, please?
patrickpatrickpatrick
force-pushed
the
add-documentation-for-accordion
branch
from
e7ac344
to
bd29713
Compare
claireashworth
approved these changes
May 14, 2024
src/components/accordion/index.md
Outdated
| @@ -65,6 +65,8 @@ There are 2 ways to use the accordion component. You can use HTML or, if you’r | |||
|
|
|||
| The accordion component uses JavaScript. When JavaScript is not available, users will see all the content displayed with the section labels as headings. | |||
|
|
|||
| If you are using HTML, you must add an 'id' attribute to the parent div of the accordion. It must be unique across the service's domain to maintain the persistent expanded state of the accordion. | |||
There was a problem hiding this comment.
suggestion That last one reads nicely 🙌🏻 Wondering if talking about 'parent div' may cause some confusion and lead to people to place that id on a <div> wrapping the accordion, rather than the one with the class="accordion" attribute. I would propose to clarify with either:
Suggested change
| If you are using HTML, you must add an 'id' attribute to the parent div of the accordion. It must be unique across the service's domain to maintain the persistent expanded state of the accordion. | |
| If you are using HTML, you must add an 'id' attribute to the root `<div>` tag of the accordion. It must be unique across the service's domain to maintain the persistent expanded state of the accordion. |
or
Suggested change
| If you are using HTML, you must add an 'id' attribute to the parent div of the accordion. It must be unique across the service's domain to maintain the persistent expanded state of the accordion. | |
| If you are using HTML, you must add an 'id' attribute to the `<div>` tag with the `accordion` class. It must be unique across the service's domain to maintain the persistent expanded state of the accordion. |
There was a problem hiding this comment.
That makes sense!
The extra documentation for the 'id' attribute that was present in the Nunjucks options table for the accordion has been added to the 'How It Works Section'. This is because if a user is just using the HTML of the accordion component, then they might not encounter the additional documentation provided for the 'id' attribute. This addresses issue #1073.
patrickpatrickpatrick
force-pushed
the
add-documentation-for-accordion
branch
from
bd29713
to
e01cb50
Compare
romaricpascal
approved these changes
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Add extra documentation for the 'id' attribute to the 'How It Works' section of the accordion component.
Why
if a user is just using the HTML of the accordion component, then they might not encounter the additional documentation provided for the 'id' attribute. This addresses issue #1073.