Taxonomy List Component #476

vanitabarrett · 2018-08-08T09:20:16Z

Trello: https://trello.com/c/PFbuMOGm/100-spike-adapt-highlight-box-component-to-work-in-a-list-of-links

We needed to work out a way to display service links (some of which are a list of highlight boxes, some are a document list) so that they are part of one HTML list. I also tried:

Adding a flag to remove the <ul> from the highlight box and document list components. However, this would require developers to a) wrap the components they're using in a list tag, and b) require them to copy over styling so the elements were displayed appropriately.

Link to the component: https://govuk-publishing-compon-pr-476.herokuapp.com/component-guide/taxonomy_list

I think we'd also be able to use this for image cards + document list

andysellick

No real problems, but as an outsider I think I have a hard time understanding this. Seems well written though.

andysellick · 2018-08-09T10:45:49Z

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
  remote: .
  specs:
-    govuk_publishing_components (9.12.0)
+    govuk_publishing_components (9.12.1)


Presumably this version bump is required for the changes you're making? Otherwise should it be in a separate PR?

This is so the PR herokuapp built - whoever last versioned the gem didn't bundle and commit the change to the Gemfile.lock so it was failing to build.

andysellick · 2018-08-09T11:20:57Z

app/assets/stylesheets/govuk_publishing_components/components/_taxonomy-list.scss

+  @include media(tablet) {
+    @include box-sizing(border-box);
+    width: 33%;
+    display: inline-block;


Not sure inline-block is needed here?

andysellick · 2018-08-09T11:22:22Z

app/views/govuk_publishing_components/components/_document_list.html.erb

@@ -8,9 +10,11 @@
  brand_helper = GovukPublishingComponents::AppHelpers::BrandHelper.new(brand)
 %>
 <% if items.any? %>
-  <ol class="gem-c-document-list<%= margin_bottom_class %><%= margin_top_class %> <%= brand_helper.brand_class %>">
+  <% unless within_multitype_list %>
+    <ol class="gem-c-document-list<%= margin_bottom_class %><%= margin_top_class %> <%= brand_helper.brand_class %>">


Will this change mean that branding can't be applied to a document list inside a taxonomy list? (I don't know if that would be a problem or not)

andysellick · 2018-08-09T11:24:01Z

app/views/govuk_publishing_components/components/_document_list.html.erb

@@ -45,5 +49,7 @@
        <% end %>
      </li>
    <% end %>
-  </ol>
+  <% unless within_multitype_list %>


This isn't hugely untidy, so this suggestion may be overkill, but have you considered passing this chunk into content_tag to output or not output this ol?

I'm not sure how this would work, can you please give an example? 🙂

I was thinking you'd wrap up the code in a block somehow and then do an if statement of some kind of either just output the block or do content_tag('ol', block). Thinking about this in more detail it's probably not appropriate for this situation, as you either want the wrapping ol or you don't - this would work better where you would want a choice of wrapping elements.

andysellick · 2018-08-09T11:34:39Z

app/views/govuk_publishing_components/components/_taxonomy_list.html.erb

+<% if highlight_box || document_list || image_cards %>
+  <ul class="gem-c-taxonomy-list" data-module="track-click">
+    <% if image_cards %>
+      <% image_cards[:items].in_groups_of(3, false) do |image_group| %>


Just seeking clarification here - why is this using in_groups_of(3 ? I can't figure it out.

Good spot, this was just left over from when I copied this code over from org pages - it's not needed any more 👍

andysellick · 2018-08-09T11:36:44Z

app/views/govuk_publishing_components/components/_taxonomy_list.html.erb

+              heading_text: image_card[:link][:text],
+              image_src: image_card[:image][:url],
+              image_alt: image_card[:image][:alt],
+              heading_level: image_card[:link][:heading_level],


Presumably it's not possible to pass through the parameters cleanly like for highlight_boxes and document_list? Is there anything we could change in the image card component to make that possible?

As this was just a spike, I didn't look too closely into it - but I'll definitely take a look now 🙂

andysellick · 2018-08-09T11:38:10Z

app/views/govuk_publishing_components/components/docs/taxonomy_list.yml

@@ -0,0 +1,85 @@
+name: Taxonomy List
+description: Wraps the highlight box and document list component in one list. This is commonly used on topic pages for links to tagged content.


Should this also mention the image card here?

andysellick · 2018-08-09T12:57:54Z

app/views/govuk_publishing_components/components/_taxonomy_list.html.erb

+      <% image_cards[:items].in_groups_of(3, false) do |image_group| %>
+        <% image_group.each do |image_card| %>
+          <li class="gem-c-taxonomy-list__item">
+            <%= render "govuk_publishing_components/components/image_card", {


Putting highlight boxes and document lists together works and makes sense, particularly when tested using Voiceover, but I'm not sure I'm convinced of the need to be able to do the same with image cards and document lists. What's the thinking behind this?

The same as for highlight boxes and document lists - we need both to be the same list for screenreaders. See the "News and Communications" section on /student-finance

andysellick · 2018-08-09T12:59:30Z

app/views/govuk_publishing_components/components/docs/taxonomy_list.yml

+            text: High needs funding
+            path: /high-needs-funding
+          metadata:
+            document_type: Guide


Could there be an example of all three together? Also if we wanted to combine one image card, one highlight box, and a document list, what would it look like - would they all stack?

Hmm, this doesn't render perfectly at the moment (the highlight boxes end up on the same line as the image cards). I've had a look into this today but it's proving tricky to fix. As we don't need this yet, I'm going to create a separate card so it doesn't block this PR.

Have you tried applying a class to the last element of each component type to provide (whatever is the flex equivalent of) clearing? Although I've had a quick look and can't see what that might be...

@andysellick Yeah, there doesn't seem to be an easy flex equivalent, annoyingly. I got a partial solution, but it then got confusing with different numbers of image cards and on mobile

Fair enough. Maybe add a note to the documentation body to point this out until we can get a fix.

vanitabarrett · 2018-08-10T14:57:26Z

@andysellick I think I've addressed all your comments / made changes - can you please re-review?

andysellick

Just some small stuff.

andysellick · 2018-08-10T15:21:02Z

app/views/govuk_publishing_components/components/docs/taxonomy_list.yml

+body: |
+  The [highlight box](/component-guide/highlight_boxes), [document list](/component-guide/document_list) and [image card](/component-guide/image_card) components are standalone components.
+
+  However, there are some use cases where we want to use both components to render one list of links. This is not possible when using the two components separately.


Reword 'both'.

andysellick · 2018-08-10T15:22:33Z

app/views/govuk_publishing_components/components/_highlight_boxes.html.erb

  inverse_class = "gem-c-highlight-boxes--inverse" if inverse
  highlight_boxes_helper = GovukPublishingComponents::Presenters::HighlightBoxesHelper.new(local_assigns)
 %>
 <% if items.any? %>
-  <ol class="gem-c-highlight-boxes" <%= "data-module=track-click" if highlight_boxes_helper.data_tracking? %>>
+  <% unless within_multitype_list %>
+    <ol class="gem-c-highlight-boxes" <%= "data-module=track-click" if highlight_boxes_helper.data_tracking? %>>


This isn't really part of this PR, but is there a way of writing this line that github will like?

I haven't been able to find much info on this - I tried a few things but nothing seemed to fix it.

Fair enough. I'll take a look at the rest shortly.

andysellick · 2018-08-10T15:42:59Z

app/assets/stylesheets/govuk_publishing_components/components/_document-list.scss

@@ -64,3 +64,7 @@
 .gem-c-document-list--top-margin {
  margin-top: $gutter-two-thirds;
 }
+
+.gem-c-document-list__multi-list {
+  width: 100%;


I've just noticed that the right edge of this (the grey dividing border) seems to overlap the page width (although I'm not sure how this will look in production). See below.

Might need to put a margin-right on it of 25px to counter the negative margin on the parent.

Good spot, I thought it didn't look quite right! 👍

andysellick · 2018-08-10T15:44:54Z

app/views/govuk_publishing_components/components/docs/taxonomy_list.yml

+            path: /high-needs-funding
+          metadata:
+            document_type: Guide
+            public_updated_at: 2016-06-27 10:29:44


Just for testing the layout I'd suggest adding another two link entries in here - so the displayed example has four highlight boxes.

andysellick · 2018-08-10T15:46:55Z

app/views/govuk_publishing_components/components/docs/taxonomy_list.yml

+            text: High needs funding
+            path: /high-needs-funding
+          metadata:
+            document_type: Guide


Fair enough. Maybe add a note to the documentation body to point this out until we can get a fix.

Wraps the highlight box and document list component in one list. This is commonly used on topic pages for taxonomy-related navigation.

Moves the branding class to the document list item instead of the ul which may not always be rendered.

vanitabarrett · 2018-08-14T09:47:40Z

@andysellick Thanks for your review - I've made all the fixes (apart from the Github error highlighting around data-module which shouldn't block this PR). Are you happy to approve?

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 8, 2018 09:20 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 77be237 to d6074d0 Compare August 8, 2018 12:21

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 8, 2018 12:21 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from d6074d0 to 731a299 Compare August 8, 2018 13:12

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 8, 2018 13:13 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 731a299 to 6117f73 Compare August 8, 2018 14:32

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 8, 2018 14:32 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 6117f73 to 0b4254b Compare August 8, 2018 14:34

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 8, 2018 14:35 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 0b4254b to aea6df4 Compare August 8, 2018 16:33

benthorner had a problem deploying to govuk-publishing-compon-pr-476 August 8, 2018 16:33 Failure

vanitabarrett force-pushed the spike-taxonomy-list branch from aea6df4 to a11ee4e Compare August 9, 2018 09:21

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 09:21 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from a11ee4e to 44f3545 Compare August 9, 2018 09:22

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 09:22 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 44f3545 to 43b5488 Compare August 9, 2018 09:26

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 09:26 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 43b5488 to 82cceac Compare August 9, 2018 10:17

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 10:17 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 82cceac to 807b476 Compare August 9, 2018 10:25

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 10:25 Inactive

vanitabarrett changed the title ~~[WIP][DO NOT MERGE] Taxonomy List Component~~ [DO NOT MERGE] Taxonomy List Component Aug 9, 2018

vanitabarrett changed the title ~~[DO NOT MERGE] Taxonomy List Component~~ Taxonomy List Component Aug 9, 2018

vanitabarrett force-pushed the spike-taxonomy-list branch from 807b476 to 4f5b2c6 Compare August 9, 2018 10:29

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 10:30 Inactive

vanitabarrett requested a review from andysellick August 9, 2018 12:14

andysellick reviewed Aug 9, 2018

View reviewed changes

vanitabarrett force-pushed the spike-taxonomy-list branch from 4f5b2c6 to acc3ec8 Compare August 9, 2018 13:19

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 13:19 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from acc3ec8 to 730d492 Compare August 9, 2018 13:21

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 14:19 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from a91cf1b to 31dae0c Compare August 9, 2018 14:21

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 14:21 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 31dae0c to 6fd66b5 Compare August 9, 2018 16:04

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 9, 2018 16:04 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 6fd66b5 to ea9c93c Compare August 10, 2018 14:07

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 10, 2018 14:07 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from ea9c93c to 3cbd918 Compare August 10, 2018 14:56

benthorner had a problem deploying to govuk-publishing-compon-pr-476 August 10, 2018 14:57 Failure

vanitabarrett force-pushed the spike-taxonomy-list branch from 3cbd918 to fa2d751 Compare August 10, 2018 14:58

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 10, 2018 14:58 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from fa2d751 to e345ea1 Compare August 10, 2018 15:17

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 10, 2018 15:17 Inactive

andysellick requested changes Aug 10, 2018

View reviewed changes

vanitabarrett force-pushed the spike-taxonomy-list branch from e345ea1 to 2c6bdc1 Compare August 14, 2018 09:23

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 14, 2018 09:23 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 2c6bdc1 to 0bbe2e1 Compare August 14, 2018 09:30

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 14, 2018 09:30 Inactive

benthorner temporarily deployed to govuk-publishing-compon-pr-476 August 14, 2018 09:44 Inactive

vanitabarrett force-pushed the spike-taxonomy-list branch from 64becca to 0bbe2e1 Compare August 14, 2018 09:44

Vanita Barrett added 2 commits August 14, 2018 09:46

Add taxonomy list component

40e9a9d

Wraps the highlight box and document list component in one list. This is commonly used on topic pages for taxonomy-related navigation.

Fix branding

6644611

Moves the branding class to the document list item instead of the ul which may not always be rendered.

vanitabarrett force-pushed the spike-taxonomy-list branch from 0bbe2e1 to 6644611 Compare August 14, 2018 09:47

benthorner deployed to govuk-publishing-compon-pr-476 August 14, 2018 09:47 View deployment

andysellick approved these changes Aug 14, 2018

View reviewed changes

vanitabarrett merged commit 9da4831 into master Aug 14, 2018

vanitabarrett deleted the spike-taxonomy-list branch August 14, 2018 09:59

vanitabarrett mentioned this pull request Aug 14, 2018

Version 9.13.0 #481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taxonomy List Component #476

Taxonomy List Component #476

vanitabarrett commented Aug 8, 2018 •

edited

andysellick left a comment

andysellick Aug 9, 2018

vanitabarrett Aug 9, 2018

andysellick Aug 9, 2018

andysellick Aug 9, 2018

andysellick Aug 9, 2018

vanitabarrett Aug 9, 2018

andysellick Aug 9, 2018

andysellick Aug 9, 2018

vanitabarrett Aug 9, 2018

andysellick Aug 9, 2018

vanitabarrett Aug 9, 2018

andysellick Aug 9, 2018

andysellick Aug 9, 2018

vanitabarrett Aug 9, 2018

andysellick Aug 9, 2018

vanitabarrett Aug 10, 2018

andysellick Aug 10, 2018

vanitabarrett Aug 10, 2018

andysellick Aug 10, 2018

vanitabarrett commented Aug 10, 2018

andysellick left a comment

andysellick Aug 10, 2018

andysellick Aug 10, 2018

vanitabarrett Aug 14, 2018

andysellick Aug 14, 2018

andysellick Aug 10, 2018

vanitabarrett Aug 14, 2018

andysellick Aug 10, 2018

andysellick Aug 10, 2018

vanitabarrett commented Aug 14, 2018

		@@ -0,0 +1,85 @@
		name: Taxonomy List
		description: Wraps the highlight box and document list component in one list. This is commonly used on topic pages for links to tagged content.

Taxonomy List Component #476

Taxonomy List Component #476

Conversation

vanitabarrett commented Aug 8, 2018 • edited

andysellick 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

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

vanitabarrett commented Aug 10, 2018

andysellick 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

vanitabarrett commented Aug 14, 2018

vanitabarrett commented Aug 8, 2018 •

edited