dwp / govuk-casa

Framework for creating simple GOVUK Collect-And-Submit-Applications
ISC License
34 stars 24 forks source link

WCAG 2.1: Support input attributes for dates and addresses #9

Closed colinrotherham closed 5 years ago

colinrotherham commented 5 years ago

This commits prepares us for WCAG 2.1 Input Purposes for User Interface Components https://www.w3.org/TR/WCAG21/#input-purposes

Each date input can now have customisable params

Using the items[] array as we're extending govukDateInput()

{{ casaGovukDateInput({
  items: [
    {
      autocomplete: "bday-day",
      attributes: {
        min: "1",
        max: "31"
      }
    },
    {
      autocomplete: "bday-month",
      attributes: {
        min: "1",
        max: "12"
      }
    },
    {
      autocomplete: "bday-year",
      attributes: {
        min: "1",
        max: "9999"
      }
    }
}) }}

Each address line can also do the same

{{ casaPostalAddressObject({
  address1: {
    autocomplete: 'address-line1'
  },
  address2: {
    autocomplete: 'address-line2'
  },
  address3: {
    autocomplete: 'address-level2'
  },
  address4: {
    autocomplete: 'address-level1'
  },
  postcode: {
    autocomplete: 'postal-code'
  }
}) }}
colinrotherham commented 5 years ago

We have a security vulnerability in node-sass due to: https://github.com/sass/node-sass/issues/2625

node-sass > node-gyp > tar

No fix is available until node-sass@5 as upgrading to node-gyp@4 (with the tar upgrade) breaks backwards compatibility.

colinrotherham commented 5 years ago

I've updated audit-resolv.json to extend the tar issue another week.

lhokktyn commented 5 years ago

This is great, thanks Colin. Would you do me a favour and just remove any changes to the package.json or package-lock.json files as we'll manage those dependency changes separately?. Don't worry about the travis checks failing as we'll handle this outside of the PR.

colinrotherham commented 5 years ago

@lhokktyn Those commits are now removed, including the tar vulnerability reminder.

lhokktyn commented 5 years ago

In both cases, I wonder if it might allow us more flexibility to place the user-specified values after the defaults, so they can override those defaults. For example, instead of:

mergeObjectsDeep(params.items[0] if params.items[0] else {}, {
      label: t('macros:dateInput.day'),
      name: params.namePrefix + '[dd]',
      id: 'f-' + params.namePrefix + '[dd]',
      value: params.casaValue.dd,
      classes: 'govuk-input--width-2 ' + (inputErrorClass if includes(fieldErrors[0].focusSuffix, '[dd]') or not hasSuffixHighlights)
}),

Use:

mergeObjectsDeep({
      label: t('macros:dateInput.day'),
      name: params.namePrefix + '[dd]',
      id: 'f-' + params.namePrefix + '[dd]',
      value: params.casaValue.dd,
      classes: 'govuk-input--width-2 ' + (inputErrorClass if includes(fieldErrors[0].focusSuffix, '[dd]') or not hasSuffixHighlights)
}, params.items[0] if params.items[0] else {}),

Clearly this would break rendering if those values were overridden with incompatible equivalents (e.g. the id needs to be just-so for the error summary to link correctly), but that would be self-sabotage so is in nobody's interest! The main advantage being that label and classes could be overridden.

What do you think?

colinrotherham commented 5 years ago

@lhokktyn Yeah I'd prefer this too, but noticed a precedent was already set to do the opposite.

I'm happy if you are? I'll switch it around.

lhokktyn commented 5 years ago

Good shout. What about introducing a common approach along the lines of:

mergeObjectsDeep({
  overridable: '...',
  things: '...',
  here: '...'
}, userProvidedParams, {
  mandatory: '...',
  things: '...',
  here: '...'
});

So in the above case we might have:

mergeObjectsDeep({
  id: 'f-' + params.namePrefix + '[dd]',
  name: params.namePrefix + '[dd]',
  value: params.casaValue.dd
}, params.items[0] if params.items[0] else {}, {
  label: t('macros:dateInput.day'),
  classes: 'govuk-input--width-2 ' + (inputErrorClass if includes(fieldErrors[0].focusSuffix, '[dd]') or not hasSuffixHighlights)
})
colinrotherham commented 5 years ago

Latest push includes the “overridable, user, mandatory” merge order feedback. Thanks 😊

lhokktyn commented 5 years ago

Merged internally (f54c402c54621d7d464fc8ca5a1d4eae6bb6e4cc). Note: reversed order of merging stated above to prevent attributes like id being overridden.