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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Apr 30, 2019

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
Copy link
Contributor Author

We have a security vulnerability in node-sass due to:
sass/node-sass#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 colinrotherham changed the title Support input attributes for dates and addresses WCAG 2.1: Support input attributes for dates and addresses Apr 30, 2019
@colinrotherham
Copy link
Contributor Author

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

@lhokktyn
Copy link
Member

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
Copy link
Contributor Author

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

@lhokktyn
Copy link
Member

lhokktyn commented May 1, 2019

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
Copy link
Contributor Author

@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
Copy link
Member

lhokktyn commented May 1, 2019

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)
})

Signed-off-by: Colin Rotherham <work@colinr.com>
Signed-off-by: Colin Rotherham <work@colinr.com>
@colinrotherham
Copy link
Contributor Author

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

@lhokktyn
Copy link
Member

lhokktyn commented May 2, 2019

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

@lhokktyn lhokktyn closed this May 2, 2019
@lhokktyn lhokktyn added the merged label May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants