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

Add summary list component #1065

Merged
merged 5 commits into from Jan 9, 2019

Conversation

@dashouse
Copy link
Contributor

commented Nov 19, 2018

This component was initially developed to allow us to build the
'check your answers' pattern.

It is mostly the same as in the original pattern with some notable differences:

  • On smaller screens it wraps by default
  • It's possible to have multiple actions

Review

Todo

  • Consider visuallyHiddenText feature instead of using html
  • Improve the options descriptions
  • Assistive technology final run through

https://trello.com/c/kh2atSrd/1612-add-summary-list-component-to-govuk-frontend-pair

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 19, 2018 Inactive

@nickcolley nickcolley force-pushed the data-list-component branch from 61fced3 to 5283ce5 Nov 22, 2018

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 22, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 22, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 22, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 22, 2018 Inactive

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

I'm wondering if we should rename the classes to something like

.govuk-summary-list__key {}
.govuk-summary-list__value {}
.govuk-summary-list__actions {}

@dashouse can we only include modifiers that we know are needed for check your answers?
Reducing the API surface is good if possible, but if you think some of these modifiers are definitely going to be used I won't block that.

Additionally, can we just always do the break wrap behaviour? I'm worried that in the real world it's not realistic for people to conditionally apply classes based on content.

This would also mean users of zoom text, would always have the actions on the left.

@dashouse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

I think there was some desire to get away from the "key value pairs" name, I'm not sure if that extends to class names too but I'm not precious about any of it.

Regarding the wrapping as default, let me talk to Joe as I think he designed it originally. The only reason I didn't do this was that the current pattern doesn't.

The modifiers were there to make it meet the needs for more than 'Check answers', although I'm not sure we need the text align one as that would be an easy modification to make for someone / could be a universal override.

p.s. Looking good, I think a few of the CSS changes you've made have changed the spacing a little. We can go through this when we're in the same room if that's easier.

@joelanman

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Just to note datalist is an existing html element so we might not want to reuse the name:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/datalist

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 23, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 26, 2018 Inactive

@fofr

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

Current screenshot for interested onlookers:
screen shot 2018-11-26 at 12 13 11

https://govuk-frontend-review-pr-1065.herokuapp.com/components/data-list

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 26, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 26, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 26, 2018 Inactive

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

I went through the API proposal with the development team and we agreed with the current approach.

API proposals discussed

Classes functionality, now:

{{ govukDataList({
  "keyClasses": "govuk-!-width-one-half",
  "valueClasses": "govuk-!-width-one-quarter",
  "actionsClasses": "govuk-!-width-one-half",
  "rows": [
    {
      "key": "Name",
      "text": "Firstname Lastname",
      "actions": [
        {
          "href": "#",
          "text": "Edit"
        },
        {
          "href": "#",
          "text": "Delete"
        }
      ]
    }
  ]
}) }}

Alternatives:

{{ govukDataList({
  "rows": [
    {
      "key": {
        classes: 'key-class',
        text: "Name"
      },
      "value": {
        classes: 'value-class',
        "text": "Sarah Philips"
      },
      "actions": {
        classes: 'actions-class',
        items: [
          {
            "href": "#",
            "text": "Change"
          }
        ]
      }
      ]
    }
  ]
}) }}

or maybe

{{ govukDataList({
  "rows": [
    {
      "keyClasses": "govuk-!-width-one-half",
      "key": "Name",
      "classes": "govuk-!-width-one-quarter",
      "text": "Firstname Lastname",
      "actionsClasses": "govuk-!-width-one-half",
      "actions": [
        {
          "href": "#",
          "text": "Edit"
        },
        {
          "href": "#",
          "text": "Delete"
        }
      ]
    }
  ]
}) }}

I think by nesting it's more verbose but allows us to allow for text/html consistently too?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 27, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 27, 2018 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Nov 27, 2018 Inactive

@dashouse dashouse changed the title WIP - Data list component WIP - Data summary list component Dec 10, 2018

@nickcolley nickcolley force-pushed the data-list-component branch from e5df553 to 562434e Dec 11, 2018

@nickcolley nickcolley force-pushed the data-list-component branch 4 times, most recently from 8adbdd6 to a817b3c Dec 14, 2018

@nickcolley nickcolley changed the title WIP - Data summary list component Add data summary list component Dec 14, 2018

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Dec 14, 2018 Inactive

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Tested in assistive technologies and they're all consistent with the Prototype Kit but I wonder if we do want this behaviour...

VoiceOver OSX:

Links read "name Change" when I expected "Change name", this might be consistent with #1096

NVDA 2018: "Change name"
JAWS 17: "Change name"

On iPhone this is worse since it doesnt read out the visually hidden text at all...

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Without more investigation and thinking I'm not sure I can make a decision on if this should ship as is, it is consistent with the GOV.UK Prototype Kit implementation.

Ideally I think we should talk to the accessibility team and try to come up with how we might resolve the general issue of our govuk-visually-hidden class (#1096).

I don't believe there's any hard deadline for this component, so it could wait until we've figured out the best way to proceed.

Alternatively we could change how this component works to use ARIA, by replacing 'visuallyHiddenText' with 'accessibleName' as follows:

{
  text: 'Change',
  accessibleName: 'Change name'  // see https://www.w3.org/TR/accname-1.1/
}
<a href="#" aria-label="Change name">Change</a>
@dashouse

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

It looks like somewhere in the code refactoring we’ve lost the styling for the summary list, with borders but without actions. This means the spacing looks like this on small screens:

screen shot 2018-12-20 at 11 03 13

I'll give this a go

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Dec 20, 2018 Inactive

@dashouse

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

Latest commit adjusts small screen spacing for actions

screen shot 2018-12-20 at 11 54 30

And increases spacing when there are no actions (with a new class govuk-summary-list--no-actions .

screen shot 2018-12-20 at 11 54 42

@dashouse dashouse force-pushed the data-list-component branch from 29cfc1b to cbee09e Dec 21, 2018

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Dec 21, 2018 Inactive

@dashouse

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@nickcolley Thanks, of course. This will just work now, just moved the margin cbee09e

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Jan 3, 2019 Inactive

@nickcolley nickcolley added this to Needs review in Design System Sprint Board via automation Jan 8, 2019

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Based on the results of #1096 I think we should stick with the current approach, we can improve this in the future if we want to, so this is ready for final review.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Jan 9, 2019 Inactive

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1065 Jan 9, 2019 Inactive

Design System Sprint Board automation moved this from Needs review to Ready to merge Jan 9, 2019

@36degrees
Copy link
Contributor

left a comment

There's one minor issue with one of the examples.

Otherwise this generally looks great 👍 Top work 👏

text: Find
- href: '#'
text: View
- href: '#'

This comment has been minimized.

Copy link
@36degrees

36degrees Jan 9, 2019

Contributor

Minor – this last link doesn't have any text, so you get an empty link at the end of the actions.

actions:
items:
- href: '#'
text: Buy

This comment has been minimized.

Copy link
@36degrees

36degrees Jan 9, 2019

Contributor

😆👌

@nickcolley nickcolley added this to the [NEXT] milestone Jan 9, 2019

@aliuk2012
Copy link
Contributor

left a comment

Amazing work @nickcolley & @dashouse!!

@nickcolley nickcolley merged commit e0c66a6 into master Jan 9, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Design System Sprint Board automation moved this from Ready to merge to Done Jan 9, 2019

@joelanman

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

great work everyone :)

@aliuk2012 aliuk2012 deleted the data-list-component branch Jan 10, 2019

@aliuk2012 aliuk2012 referenced this pull request Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.