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

Remove custom attribute on status in task list example #4415

Closed
wants to merge 1 commit into from

Conversation

frankieroberto
Copy link
Contributor

Custom attributes aren't currently supported on the status within the task list, only for the overall task list itself.

We could chose to support custom attributes on other elements within the task list (such as the status, title and hint) – but I'm not sure what the convention is for this across govuk-frontend?

Custom attributes aren't currently supported on the status within the task list, only for the overall task list itself.

We could chose to support custom attributes on other elements within the task list (such as the status, title and hint) – but I'm not sure what the convention is for this across govuk-frontend?
@frankieroberto
Copy link
Contributor Author

frankieroberto commented Oct 31, 2023

I spotted this when working on the ruby implementation of the task list. Any thoughts on if/how custom attributes should be supported on lower-level elements within components?

@colinrotherham
Copy link
Contributor

Ah that's odd, Is it only the Ruby implementation that's affected?

Custom attributes aren't currently supported on the status within the task list…

I'm seeing the attributes being rendered here:
http://govuk-frontend-review.herokuapp.com/components/task-list/custom-attributes/preview

@colinrotherham
Copy link
Contributor

Forgot to add, it came up during code review to pass everything into govukTag(item.status.tag)

Custom attributes

For comparison, form elements and hints always add the exact params including attributes

{{ govukHint({
  id: hintId,
  classes: params.hint.classes,
  attributes: params.hint.attributes,
  html: params.hint.html,
  text: params.hint.text
}) | indent(2) | trim }}

But the phase banner govukTag() only supports text, html and classes not attributes

@frankieroberto
Copy link
Contributor Author

@colinrotherham:

Ah that's odd, Is it only the Ruby implementation that's affected?

No, this is about the Nunjucks implementation.

Custom attributes aren't currently supported on the status within the task list…

I'm seeing the attributes being rendered here: http://govuk-frontend-review.herokuapp.com/components/task-list/custom-attributes/preview

That's only on overall list container, not the item status?

If we want to support custom attributes (as well as custom classes) on some or all of the item, status or title, then we'll have to update the Nunjucks macro, I think, and add some extra tests to cover it.

I think generally across the Design System, it's not super consistent whether sub-components will accept attributes in the nunjucks macros or not. I guess they’ve only been added where there’s been call for it?

The Summary List seems to accept attributes on the items, card and item, but not on rows, action.items, title or actions (all of which accept classes). 🤷

@frankieroberto
Copy link
Contributor Author

Opps I made a mistake - the attribute is being added to the tag, not the status, and there’s an existing test for it! 🤦 I somehow misread that completely.

@frankieroberto frankieroberto deleted the patch-4 branch October 31, 2023 13:53
@colinrotherham
Copy link
Contributor

Ah sorry @frankieroberto

Good points though regarding inconsistencies in passing params to nested components

Probably another reason a Nunjucks "merge objects" filter or global would be handy

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.

2 participants