-
Notifications
You must be signed in to change notification settings - Fork 109
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
Support form submission from header account items #1155
base: header-breaking-changes
Are you sure you want to change the base?
Conversation
44be36d
to
e39fa33
Compare
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.
Nice!
@@ -81,6 +81,10 @@ | |||
<a class="nhsuk-header__account-link" href="{{ item.href }}"> | |||
{{ _accountItem(item) }} | |||
</a> | |||
{% elif item.action %} | |||
<button class="nhsuk-header__account-button" formaction="{{ item.action }}" formmethod="{{ item.method or "post" }}"> |
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.
Must admit I’ve never come across the formaction
or formmethod
attributes for buttons, but they’re in the spec and look like they’re well supported?
Does still need a separate form
element though, as you say.
I did wonder if it’d be simpler for the nunjucks to generate a separate form tag for each of the buttons (in most cases it’d be be log out?) - but then I guess in production you might also have extra hidden inputs for cross-site request forgery (CSRF) tokens or whatever?
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.
Precisely. This is the corresponding (Rails) implementation we currently have on Mavis:
<span class="app-header__account-item">
<form class="button_to" method="post" action="/logout">
<input type="hidden" name="_method" value="delete" autocomplete="off">
<button class="app-header__account-button" type="submit">Log out</button>
<input type="hidden" name="authenticity_token" value="XXX" autocomplete="off">
</form>
</span>
Unsure if adding a <form>
in the Nunjucks macro if any button includes an action (or for each item that has an action) is helpful or not. As Nunjucks is mostly used by prototypes, maybe it is?
@@ -81,6 +81,10 @@ | |||
<a class="nhsuk-header__account-link" href="{{ item.href }}"> | |||
{{ _accountItem(item) }} | |||
</a> | |||
{% elif item.action %} | |||
<button class="nhsuk-header__account-button" formaction="{{ item.action }}" formmethod="{{ item.method or "post" }}"> |
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.
Should this have ‘role=link‘ so that it behaves more like how it looks (eg will respond to “click link log out” for voice control)? 🤔
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.
I quickly peeked at the spec, and this role seems aimed at those using non-semantic elements for hyperlinks; a button is a button, no?
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.
I'm not sure. The NHS button adds role="button"
to links that are styled as buttons, and the GOVUK button additionally adds some javascript so that links-styled-as-buttons get clicked if the spacebar is hit when they’re focused.
Neither the GOVUK nor NHS frontend current supports buttons-styled-like-links at the moment, but there’s a backlog discussion about it here: alphagov/govuk-design-system-backlog#159
In our case, maybe it’s a bit less relevant as the "Log out" item in the header kinda looks half-way between a link and button (especially if the underlines get removed in #1136?
Description
Some items in the header account area, but most likely any ‘Log out’ button, will require form submission rather than linking to a page.
This PR adds support for having button items (in addition to having a link or only text), and adds the following 2 options:
[]item.action
– The form action, for example"/log-out"
[]item.method
– The form method, optional and defaults to"post"
If you provide
item.action
, a button will be used instead. Usingitem.href
will override anyitem.action
parameter.These options currently presume the presence of a surrounding
<form>
; perhaps we should add a<form>
element if you use one or moreitem.action
parameters?The button is styled to look and behave like a link:
If this PR is accepted, we’d need to update the service manual guidance… and possibly add some information around when you might want to use a button instead of a link.
Checklist