Add keyword arguments for all macros

[solidgoldpig]

Also, switch from text v html to text plus safe

[NickColley]

Hey @solidgoldpig !

Thanks for spending the time to put this together, we're still not sure if this is the approach we want to take to solve this issue.

I think if we were to go down this route we'd want to have some form of being able to set a default via a global. Since one of the biggest concerns is that users that are prototyping may find this behaviour hard to understand.

[solidgoldpig]

The introduction of keyword arguments allows the following approaches to get the same result

(NB. including the putative html deprecation in this PR is a red herring - mea culpa - since it doesn't affect the action of the keyword args)

1. Standard text
{{ govukHint({
  text: 'National Insurance number'
}) }}

2. Text with keyword argument
{{ govukHint(text='National Insurance number') }}

3. Text with implicit string passed as params
{{ govukHint('National Insurance number') }}

and for unescaped html

1. Standard safe unescaped html
{{ govukHint({
  text: '<b>National Insurance</b> number',
  safe: true
}) }}

2. Safe unescaped html with keyword arguments
{{ govukHint(text='<b>National Insurance</b> number', safe: true) }}

3. Safe unescaped html with implicit string passed as params and keyword arguments
{{ govukHint('<b>National Insurance</b> number', safe: true) }}

So what, you might ask?

Personally, when I'm prototyping I prefer to have as flat a data structure as possible:

foo('bar')  > foo({text: 'bar'}

However, the real strength of this approach is that it allows more compact macros of the call, while preserving all the params passed except for those explicitly overridden.

1. Previous hint implementation in radios (and checkboxes)
  {{ govukHint({
    id: hintId,
    classes: params.hint.classes,
    attributes: params.hint.attributes,
    html: params.hint.html,
    text: params.hint.text
  }) | indent(2) | trim }}

2. New hint implementation
  {{ govukHint(
    params.hint,
    id=hintId
  ) | indent(2) | trim }}

This approach could also make it easier to pass fieldset legends (pass the legend directly to radios, checkboxes, select etc and mix it in to the legend)

I did something like this for the fieldset role which in the current implementation prevents passing any other attributes

1. Previous fieldset implementation in govukDateInput
  {% call govukFieldset({
    describedBy: describedBy,
    classes: params.fieldset.classes,
    attributes: {
      role: 'group'
    },
    legend: params.fieldset.legend
  }) %}

2. New fieldset implementation
  {% call govukFieldset(
    params.fieldset,
    describedBy=describedBy,
    role='group'
  ) %}

html v safe

I came across several places where html was being used even though it didn't need to.

I can imagine people just using html by default. Mind you, I can imagine, people just sticking safe=true and forgetting about it.

My big question here is, why do labels, hints, legends and summary descriptions and title actually need escaping in the first place? I'm not saying we shouldn't be cautious, but how often does user content end up in those elements? Is there not a case that the default should be safe mode for those?

My rationale for text + safe is that the API stays the same, safe rings the klaxon that something "unusual" is going on. I appreciate that the word safe itself may conjure up the exact opposite impression than intended, but it is Nunjuck's terminology ;)

[solidgoldpig]

The above approach is predicated on doing it all in Nunjucks.

If you could mandate a bare minimum bit of code to add such a global

const setObject = (...args) => {
  const objArgs = args.slice()
  objArgs.unshift({})
  return Object.assign.apply(null, objArgs)
}
nunjucksEnv.addGlobal('setObject', setObject)

it 's trivial to do work around Nunjuck's bizarre intransigence when it comes to updating object properties

{% set params = setObject(params, { foo: 'bar' }, { foo: 'baz'}) %}

or even

{% set params = setObject({ foo: 'bar' }, params) %}

This is arguably much more powerful.

[NickColley]

Hey @solidgoldpig

This PR is trying to do two things:

1. Change the Public API for the macros
2. Address the issues around XSS described in Make it clear that the "html" attribute can be unsafe #514

For 2. we have decided we will not be changing this behaviour right now before launch, and will instead try to document the current behaviour and risks.

If this changes we'll update the thread here: #514

For 1. we're not sure what problem you're trying to solve, so if you could put together a proposal (or even an issue on this repo) we'd love to figure that out with you.

I'm going to close this out but will catch up with you when/if we do any work around here to get your thoughts.