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 internal developer docs to use new configuration guidance #5516

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

domoscargin
Copy link
Contributor

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.

@domoscargin domoscargin added documentation User requests new documentation or improvements to existing documentation javascript labels Nov 19, 2024
Copy link

github-actions bot commented Nov 19, 2024

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 118.41 KiB
dist/govuk-frontend-development.min.js 42.7 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 92.81 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 87.12 KiB
packages/govuk-frontend/dist/govuk/all.mjs 1.25 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 1.74 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 118.4 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 42.69 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB
packages/govuk-frontend/dist/govuk/init.mjs 7.13 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 82.57 KiB 40.4 KiB
accordion.mjs 26.58 KiB 13.41 KiB
button.mjs 9.09 KiB 3.78 KiB
character-count.mjs 25.39 KiB 10.9 KiB
checkboxes.mjs 7.81 KiB 3.42 KiB
error-summary.mjs 10.99 KiB 4.54 KiB
exit-this-page.mjs 20.2 KiB 10.34 KiB
header.mjs 6.46 KiB 3.22 KiB
notification-banner.mjs 9.35 KiB 3.7 KiB
password-input.mjs 18.24 KiB 8.33 KiB
radios.mjs 6.81 KiB 2.98 KiB
service-navigation.mjs 6.44 KiB 3.26 KiB
skip-link.mjs 6.4 KiB 2.76 KiB
tabs.mjs 12.04 KiB 6.67 KiB

View stats and visualisations on the review app


Action run for f3e4031

Copy link
Member

@romaricpascal romaricpascal left a 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).

docs/contributing/coding-standards/component-options.md Outdated Show resolved Hide resolved
docs/contributing/coding-standards/component-options.md Outdated Show resolved Hide resolved
docs/contributing/coding-standards/component-options.md Outdated Show resolved Hide resolved
docs/contributing/coding-standards/component-options.md Outdated Show resolved Hide resolved
Comment on lines 33 to 34
class="my-component"
data-module="my-component"
Copy link
Member

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:
Copy link
Member

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:

Suggested change
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).
Copy link
Member

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:

Suggested change
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'
}
}

@domoscargin
Copy link
Contributor Author

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

@domoscargin domoscargin marked this pull request as ready for review November 25, 2024 08:34
@domoscargin domoscargin requested a review from a team November 25, 2024 08:35
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Cheers for the updates, and thanks for your patience waiting for the re-review. Spotted a little typo in the last example. Once that's sorted, we're good to go, I think 😊

docs/contributing/coding-standards/component-options.md Outdated Show resolved Hide resolved
Include introduction to building JavaScript components.

Push support option to bottom of doc to encourage teams to build themselves.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-5516 January 7, 2025 14:42 Inactive
@domoscargin domoscargin mentioned this pull request Jan 8, 2025
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Thanks for that final update. I think we can merge it without waiting for the release as the code for ConfigurableComponent is already on main 😊

Agree that we'll need a little think of how much this piece of doc should be here vs on the Frontend Docs as well. I think it's mostly the Nunjucks parameters that we can't document there, though 🤔

@domoscargin domoscargin merged commit 9b31d39 into main Jan 9, 2025
50 checks passed
@domoscargin domoscargin deleted the bk-config-docs branch January 9, 2025 09:36
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 javascript
Projects
Status: Done 🏁
Development

Successfully merging this pull request may close these issues.

Update internal docs on configuration options
3 participants