-
Notifications
You must be signed in to change notification settings - Fork 333
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 internal developer docs to use new configuration guidance #5516
base: main
Are you sure you want to change the base?
Conversation
Include introduction to building JavaScript components. Push support option to bottom of doc to encourage teams to build themselves.
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 1c0fcaa |
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.
Strong linking game! Makes sense not to duplicate stuff we've already written on the Frontend Docs. I've added a couple of suggestions as I was reading the content, would be good to see what other people think as well (@querkmachine especially, as I think they were involved in the original piece of documentation).
class="my-component" | ||
data-module="my-component" |
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.
question As these are internal docs, could we stick with the 'govuk' prefix? Maybe it's a matter of changing the introduction for the example, leading with something like:
'For example, this is how the Accordion does it for its rememberExpanded
option:'
{%- if params.rememberExpanded %} data-remember-expanded="{{ params.rememberExpanded | escape }}"{% endif %}> | ||
... | ||
</div> | ||
``` | ||
|
||
The above code checks for the existence of the `rememberExpanded` parameter. If the parameter is present it adds the `data-remember-expanded` attribute. It then passes in the developer's defined value, running it through [Nunjucks' native `escape` filter](https://mozilla.github.io/nunjucks/templating.html#escape-aliased-as-e) to make sure that values can't break our HTML. | ||
|
||
We can now call the Accordion's Nunjucks macro with our new `rememberExpanded` parameter: | ||
We can now call the component's Nunjucks macro with our new `rememberExpanded` parameter: |
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.
issue The example following this statement still talks about the 'Accordion'.
If we switch the introduction of the previous example to present the Accordion's template as an example, we might be able to do something similar here:
We can now call the component's Nunjucks macro with our new `rememberExpanded` parameter: | |
With the previous code in the Accordion's template, this is how a page may set a specific value for the `rememberExpanded` option: |
} | ||
} | ||
``` | ||
When passing configuration into your component's constructor, you can use nested objects to group options. This isn't possible when using data attributes, which only accept strings. Instead, you should follow our [naming conventions for data attributes](https://frontend.design-system.service.gov.uk/building-configurable-components/#receiving-configuration-from-data-attributes). |
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.
suggestion The "This isn't possible" reads as if you won't be able to set nested options through data attributes. Maybe we can spin this more positively with:
When passing configuration into your component's constructor, you can use nested objects to group options. This isn't possible when using data attributes, which only accept strings. Instead, you should follow our [naming conventions for data attributes](https://frontend.design-system.service.gov.uk/building-configurable-components/#receiving-configuration-from-data-attributes). | |
When passing configuration into your component's constructor, you can use nested objects to group options. | |
Data attributes only accept strings, but our [naming conventions for data attributes](https://frontend.design-system.service.gov.uk/building-configurable-components/#receiving-configuration-from-data-attributes) allow you to set options in nested objects using a dot `.` separator for each level of nesting. For example `data-i18n.showLabel="Show"` will set the same option as the following constructor argument: | |
```json | |
{ | |
i18n: { | |
showLabel: 'Show' | |
} | |
} |
80bdf37
to
1703257
Compare
1703257
to
b5cf65e
Compare
b5cf65e
to
2c5f100
Compare
b37a00c
to
1c0fcaa
Compare
@romaricpascal I've addressed all your comments - thanks! I still think as a larger issue, ALL of this documentation should be in the frontend docs - it feels a bit arbitrary as to what's in these docs vs what's on the site. But that's a bigger job than the scope of this PR! |
Fixes #5501
Considerations
There is considerable overlap between these docs and the new docs on the website. I've mostly taken the approach of favouring the website, cutting out content and instead linking out to relevant bits of the site guidance, but the guidance here was really nice, and there are some bits that now feel a bit orphaned (ie: everything to do with Nunjucks).
I'd favour moving all the topics from this doc to the site and having a single documentation source.