DP-5343 Callout Grid #578

legostud · 2017-08-31T17:45:57Z

Description

Refactoring the Key Actions pattern to work within the Action Finder.
Updated the Action Finder to use a Comp Heading and Key Actions

Related Issue / Ticket

https://jira.state.ma.us/browse/DP-5343

Steps to Test

Compare http://localhost:3000/?p=organisms-action-finder to http://mayflower.digital.mass.gov/?p=organisms-action-finder
Compare http://localhost:3000/?p=organisms-action-finder-without-background to http://mayflower.digital.mass.gov/?p=organisms-action-finder-without-background
Note that nothing appears different.

Impacted Areas in Application

Key Actions - hiding the heading with a new 'hiddenHeading` variable
Key Actions - Has a new id variable
Action Finder - swapped out the heading for a Comp Heading (twig and json)
Action Finder - changed the links to render using Key Actions
Action Finder - added a labelledby attribute to the h3 headings.

@todo

Should eventually replace the hard coded h3 with a restyled Comp Heading contained in the Key Actions pattern

… throughout page (#518) * Added an aria-labelledby for the key actions using compHeading.id * Added id to the key-actions.json * Added compHeading.id to the sidebar-heading "Key Agencies" * Removed the aria label from the sidebar-heading * Updated the comp-heading with modifier class --sidebar * Moved the 'after' under the --sidebar & .sidebar * Add modifiers --centered and --sidebar to the comp-header.twig * Moved the sidebar modifier in the comp-heading.twig * Removed the compHeading.id from the key-actions.twig * Removed the center modifiere to h3 header in comp-heading.twig * DP-3232 - Comp Heading - updates * DP-3232 - Comp Heading - reverting level headings * DP-3232 - Key Actions - populating aria-labelled by and removing sidebarheading

jesconstantine

Great work @legostud -- I looooove using the key actions as a callout grid. I know you did a lot of work with a lot of care to make this backwards compatible and I appreciate it!

I just want to confirm my understanding with the questions below. Maybe you can walk me through it during scrum on Monday?

Thanks :)

jesconstantine · 2017-09-23T11:18:36Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

-      <h2 class="ma__action-finder__title">
-        {{ actionFinder.title }}
-      </h2>
+      {# Backward compatible with 5.7 - replaced h2 with compHeading #}


jesconstantine · 2017-09-23T11:40:12Z

styleguide/source/_patterns/03-organisms/by-author/key-actions.twig

@@ -3,13 +3,26 @@
  {% set keyActions = keyActions|merge({"compHeading": { "title": keyActions.sidebarHeading.title, "sidebar" : true }}) %}
 {% endif %}

-<section class="ma__key-actions">
+{# fallback to compHeading Id value #}
+{% set keyActionId = keyActions.compHeading.id %}


Should we check for keyActions.compHeading.id here first since it's optional?

jesconstantine · 2017-09-23T11:53:38Z

styleguide/source/_patterns/03-organisms/by-author/key-actions.twig

+    <div 
+      class="ma__key-actions__items" 
+      {% if keyActionId %}
+        aria-labelledby="{{ keyActionId }}"


I remember talking about the heading / labelledby id stuff, just want to make sure I understand what's happening here:

When key actions is used as a standalone component, it will generally have a compHeading (visual or semantic) with an id, so that is the default / fallback value of keyActionId -- a variable which refers to the id of the thing that describes it.

When key actions is included as a child component (i.e. actionFinder), it does not need to use its compHeading because the parent component could have its own heading and can therefor pass in an id for that heading as keyActions.id (i.e. keyActions|merge({"id": "all-" ~ actionFinder.id, }). This variable assignment happens after the fallback so it will always take precedent if it exists.

Am I understanding this correctly?

jesconstantine · 2017-09-23T11:55:46Z

styleguide/source/_patterns/03-organisms/by-author/key-actions.twig

  <div class="ma__key-actions__container">
    {% if keyActions.compHeading %}
-      {% set compHeading = keyActions.compHeading %}
+      {% set compHeading = keyActions.compHeading|merge({"id": keyActionId}) %}


Can you explain why this is necessary? (I know we talked about it!) If there is a compHeading.id don't we always want to use that? And if there isn't a compHeading.id isn't it likely that the id was passed in because there is a heading in the parent component that is including this whose id is used in aria-labelledby?

What we want to always use is the keyActions.id value. If that hasn't been passed then we fallback to the compHeading.id. If that hasn't been passed then we fall back to a blank id, because unlabeled is better than mis-labeled.

jesconstantine · 2017-09-23T11:59:46Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

-      {% if actionFinder.featuredHeading %}
-        <h3 class="ma__action-finder__category">{{ actionFinder.featuredHeading }}</h3>
-      {% endif %}
-      <div class="ma__action-finder__items">


I saw the backwards-compatible scss to support this selector - thank you!

jesconstantine · 2017-09-23T12:01:37Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

-          {% endif %}
-        {% endfor %}
+      <div class="ma__action-finder__all-items">
+        {% set keyActions = { "links": actionFinder.links } %}


This ensures that the data structure can stay the same - nice!

jesconstantine · 2017-09-23T12:12:05Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

+          <h3 class="ma__action-finder__category" id="featured-{{ actionFinder.id }}">{{ actionFinder.featuredHeading }}</h3>
+          {% set keyActions = keyActions|merge({"id": "featured-" ~ actionFinder.id, }) %}
+        {% endif %}
+        {% include "@organisms/by-author/key-actions.twig" %}


What's the id for keyActions if we don't have a featuredHeading here?

ah, now I get the other comment. I'll pass the main title's id if we don't have a featured Heading.

jesconstantine · 2017-09-23T12:17:36Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

-        {% endfor %}
+      <div class="ma__action-finder__all-items">
+        {% set keyActions = { "links": actionFinder.links } %}
+        {% if actionFinder.generalHeading and actionFinder.featuredLinks %}


Why wouldn't we display the generalHeading regardless of whether or not there were featuredLinks?

It was a design decision. Did really make sense to say "All ..." when there aren't "featured" links above it. The heading just adds confusion.

jesconstantine · 2017-09-23T12:22:11Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

-        {{ actionFinder.title }}
-      </h2>
+      {# Backward compatible with 5.7 - replaced h2 with compHeading #}
+      {% set compHeading = actionFinder.compHeading ? : {"title": actionFinder.title, "color":"yellow","id": ""} %}


Don't we want the title heading to have an id since that could potentially be passed down to the keyActions aria-labelledby (i.e. in the else clause for the general items)

The aria-labelledby id would be the sub heading not the this title.

jesconstantine · 2017-09-23T12:25:02Z

styleguide/source/_patterns/03-organisms/by-author/action-finder.twig

+          <h3 class="ma__action-finder__category" id="all-{{ actionFinder.id }}">{{ actionFinder.generalHeading }}</h3>
+          {% set keyActions = keyActions|merge({"id": "all-" ~ actionFinder.id, }) %}
+        {% else %}
+          {% set keyActions = keyActions|merge({"id": actionFinder.id, }) %}


This actionFinder.id doesn't currently correspond to a heading, does it?

legostud · 2017-09-29T12:25:14Z

I think I've address all of the issues here. please feel free to review again

legostud added 5 commits August 31, 2017 13:34

DP-5343 - Comp Heading - removed extra whitespace in content

0c84e79

DP-5343 - Key Actions - Adding id and option to hide the heading

3c45abe

DP-5343 - Action Finder - adding Key Actions and Comp Heading.

80202b6

DP-5343 - Action Finder - Removed filter class

78fe66b

legostud requested a review from jesconstantine August 31, 2017 17:46

legostud added 2 commits September 20, 2017 10:48

Merge remote-tracking branch 'origin/dev' into DP-5343-CalloutGridsv2

a6cb9d3

Merge branch 'dev' into DP-5343-CalloutGridsv2

df7ce3d

jesconstantine requested a review from isaacchansky September 21, 2017 11:59

jesconstantine reviewed Sep 23, 2017

View reviewed changes

legostud added 2 commits September 28, 2017 16:00

PR review updates

ce9cc26

DP-5343 - Key Actions - fixing ID logic

d192c42

jesconstantine added this to the 6.0.0 milestone Oct 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DP-5343 Callout Grid #578

DP-5343 Callout Grid #578

legostud commented Aug 31, 2017

jesconstantine left a comment

jesconstantine Sep 23, 2017

jesconstantine Sep 23, 2017

jesconstantine Sep 23, 2017

legostud Sep 28, 2017

jesconstantine Sep 23, 2017

legostud Sep 28, 2017

jesconstantine Sep 23, 2017

jesconstantine Sep 23, 2017

jesconstantine Sep 23, 2017

legostud Sep 28, 2017

jesconstantine Sep 23, 2017

legostud Sep 28, 2017

jesconstantine Sep 23, 2017 •

edited

Loading

legostud Sep 28, 2017

jesconstantine Sep 23, 2017

legostud Sep 28, 2017

legostud commented Sep 29, 2017

DP-5343 Callout Grid #578

Are you sure you want to change the base?

DP-5343 Callout Grid #578

Conversation

legostud commented Aug 31, 2017

Description

Related Issue / Ticket

Steps to Test

Impacted Areas in Application

jesconstantine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesconstantine Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legostud commented Sep 29, 2017

jesconstantine Sep 23, 2017 •

edited

Loading