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 documentation for component options #1181

Merged
merged 6 commits into from
Feb 13, 2019
Merged

Update documentation for component options #1181

merged 6 commits into from
Feb 13, 2019

Conversation

36degrees
Copy link
Contributor

This corrects a bunch of issues with the documentation for our components, off the back of some exploratory work into testing the documentation – which is really hard, and not something we're ready to merge yet.

See the individual commits for details.

The breadcrumbs accept attributes on each individual 'crumb', but this isn't documented.

The select accepts attributes on each individual <option>, but this isn't documented.
describedBy is listed as an option for both the character count and the textarea component, but in both cases this is actually a variable used only within the component, that cannot be passed in as part of `params`.
- Use 'params' instead of 'arguments'.
- Add missing html and text options for the 'meta' section of the footer.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1181 February 8, 2019 15:03 Inactive
@@ -3,17 +3,17 @@
<div class="govuk-width-container {{ params.containerClasses if params.containerClasses }}">
{% if params.navigation %}
<div class="govuk-footer__navigation">
{% for item in params.navigation %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests have failed on this change, but it looks sensible so not sure why.

@kr8n3r
Copy link

kr8n3r commented Feb 12, 2019

outputed class is govuk-footer__list--columns-
because
{% if nav.columns %}
govuk-footer__list--columns-{{ item.columns }}
{% endif %}

@36degrees
Copy link
Contributor Author

Good catch, thanks @igloosi 👍

{% set listClasses %}
{% if item.columns %}
{% if nav.columns %}
Copy link
Member

Choose a reason for hiding this comment

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

The below line will just need updating to {{ nav.columns }} and then the tests will pass again

Currently we reuse the `item` variable within a nested for loop (`{% for item in params.navigation %}` is the outer loop, `{% for item in item.items %}` is the inner loop), which makes it difficult to understand what is going on (I'm surprised this works!)

This updates the outer loop to use `{% for nav in params.navigation %}` instead.
Radios do not accept a `name` for each radio button, as unlike checkboxes radios within the same fieldset should always have the same name.
- Add undocumented 'title' option
- Add undocumented `attributes` option for each panel
- Reformat panel options to be represented as a nested object
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.

These are great catches, thanks @36degrees 👍

@kr8n3r
Copy link

kr8n3r commented Feb 12, 2019

technically I think this should be a fix in the changelog, but it depends on the regard of macro-options.json as being part of the public API

@NickColley
Copy link
Contributor

macro-options is not documented so it's not part of the public api, updating the template.njk did not change anything for users or the api.

So given that I'd argue these are all internal changes?

@kr8n3r
Copy link

kr8n3r commented Feb 12, 2019

dilemma solved

@36degrees 36degrees merged commit de94a02 into master Feb 13, 2019
@36degrees 36degrees mentioned this pull request Mar 5, 2019
@aliuk2012 aliuk2012 deleted the update-options branch May 9, 2019 06:29
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.

5 participants