Update taxonomy sidebar design #1222

steventux · 2017-12-11T18:16:48Z

https://trello.com/c/zzot5dZq/157-taxonomy-sidebar-add-link-to-collections
https://trello.com/c/NtD5rC4S/121-taxonomy-sidebar

This combines the changes from #1200 to make the taxonomy sidebar styles similar to the related-navigation component and also the feature of rendering collections links below the taxon links.

We are developing a new related links component in government-frontend. We need to bring the taxonomy sidebar component design in line with the new related links.

'Part of' links are moving to the sidebar navigation, so they need to be present in the taxonomy sidebar.

maxgds

We need to avoid making global rather than targeted changes to links etc on the page.

maxgds · 2017-12-12T10:37:55Z

app/assets/stylesheets/govuk-component/_taxonomy-sidebar.scss

+    margin-bottom: $gutter-one-third;
+  }
+
+  a {


We usually wouldn't style an "a" directly. Sometimes we are forced to, but we absolutely need to ensure we're not styling all a tags on the page, so
.taxon-description a { blah }
or something may be necessary - ideally though the anchor would have a styling class such as .govuk-taxonomy-sidebar__link

These seem to be nested within the selector at the top, but massive 👍 to always avoiding styling base elements if possible.

We should only be doing this if there's no other way as it directly conflicts with our naming conventions that allow us to have confidence with components across so many applications.

https://github.com/alphagov/govuk_publishing_components/blob/master/docs/component_conventions.md

Specifically we're using BEM and namespacing

maxgds · 2017-12-12T10:39:05Z

app/assets/stylesheets/govuk-component/_taxonomy-sidebar.scss

+    text-decoration: none;
+  }
+
+  nav {


again, this needs to be scoped

maxgds · 2017-12-12T10:39:22Z

app/assets/stylesheets/govuk-component/_taxonomy-sidebar.scss

+    margin-bottom: $gutter;
+  }
+
+  li {


scoping needed

maxgds · 2017-12-12T10:40:49Z

app/assets/stylesheets/govuk-component/_taxonomy-sidebar.scss

+  li {
+    padding: 0;
+    list-style-type: none;
+    margin-bottom: $gutter-one-third;
  }

  ul {


steventux · 2017-12-12T11:18:42Z

@maxgds @NickColley I'm confused about requested changes, could you clarify what, if anything, needs changing on this PR please?

maxgds · 2017-12-12T11:21:52Z

I hadn't noticed the things I commented on were nested, as such they are they are safe to go out.

However, we shouldn't really be applying the css directly to elements even when embedded. I'd say mark it as tech debt, put it on the board in the tech debt column Vanita created and let this go out as-is for now.

We've discussed comments

NickColley

This isn't techincal debt, we should just make the updates in this review, we follow these conventions to make sure do not break GOV.UK.

maxgds · 2017-12-12T11:39:52Z

The diff is confusing - Steve hasn't actually introduced this, hence my comment. And it's relatively safe as it's nested in the scss, not completely bare.

steventux · 2017-12-12T11:45:48Z

@NickColley forgive my ignorance, but it's not clear which changes are necessary to make this PR acceptable production-ready component code.
It seems like some unconventional CSS scoping from previous work is being conflated with the intended scope of this PR (some cursory design changes and adding the collections links, none of which add unscoped styles).

NickColley · 2017-12-12T12:24:20Z

@steventux if we're writing new CSS we should be using the correct naming conventions (linked above). Old code is fine, and is covered by https://trello.com/c/SCio6yb4/45-static-components-need-updating-to-new-component-principles

steventux · 2017-12-12T12:48:02Z

My bad - had cherry-picked a commit from #1200, was unaware this contained the offending CSS

steventux · 2017-12-12T14:19:04Z

@NickColley @maxgds hopefully this e1dc506 sorts out the element selectors. Let me know if I'm off with the BEM naming.

maxgds

Personally I'd call govuk-taxonomy-sidebar__items govuk-taxonomy-sidebar__list (singular rather than plural label) but happy to approve this now.

steventux · 2017-12-13T08:50:21Z

@NickColley I've amended the component styles so they follow the BEM naming convention, apologies @maxgds for not understanding the original review comments.
Anything else to do on this PR?

NickColley

Thanks for making these changes, sorry for the miscommunication earlier - I should have caught up with you in person rather than confusing things in comments.

I've added a non blocking note around how you can think about your classes.

If your classes do not understand what it presents, they we can make them less verbose, since a heading is a heading, regardless if it's for a taxon section or X section.

In the same way that we're trying not to have specific styles for a page, for example .contacts-page-heading, this logic can be applied to the components too.

This could be done later and does not block the PR, lemme know if I did not make sense and you want to chat in person. 😄

NickColley · 2017-12-15T11:06:03Z

app/assets/stylesheets/govuk-component/_taxonomy-sidebar.scss

+  padding-top: $gutter-half;
+}
+
+.govuk-taxonomy-sidebar__related-heading,


Non blocking:

These classes look to be the same, if we decouple how we present parts of this component from the semantics of this component, could these be change to a single '.govuk-taxonomy-sidebar__heading`?

steventux · 2017-12-15T12:56:12Z

Thanks @NickColley your comment makes sense to me, I've renamed the heading and link classes to simplify: dd42118#diff-b2af3eb92cd75ba5d8c0d1ab14ce7424R7
I will think of a better name for the collections classes as it suffers from the same naming problem.

I've addressed Nick's comments #1222 (comment)

Update design of taxonomy sidebar

50ec798

We are developing a new related links component in government-frontend. We need to bring the taxonomy sidebar component design in line with the new related links.

fofr temporarily deployed to govuk-static-pr-1222 December 11, 2017 18:16 Inactive

steventux force-pushed the update-taxonomy-sidebar-design branch from b1efbf2 to 3ce8eb2 Compare December 11, 2017 18:24

fofr temporarily deployed to govuk-static-pr-1222 December 11, 2017 18:24 Inactive

steventux force-pushed the update-taxonomy-sidebar-design branch from 3ce8eb2 to 26f5dc9 Compare December 12, 2017 08:46

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 08:47 Inactive

Steve Laing added 2 commits December 12, 2017 09:03

Render collections links in taxonomy sidebar

5c86d28

'Part of' links are moving to the sidebar navigation, so they need to be present in the taxonomy sidebar.

Add collections data to taxonomy sidebar docs

179473f

steventux force-pushed the update-taxonomy-sidebar-design branch from 26f5dc9 to 179473f Compare December 12, 2017 09:04

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 09:04 Inactive

maxgds previously requested changes Dec 12, 2017

View reviewed changes

NickColley previously requested changes Dec 12, 2017

View reviewed changes

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 13:31 Inactive

steventux force-pushed the update-taxonomy-sidebar-design branch from 5beb336 to c1da5eb Compare December 12, 2017 13:43

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 13:43 Inactive

steventux force-pushed the update-taxonomy-sidebar-design branch from c1da5eb to fa16a21 Compare December 12, 2017 14:14

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 14:14 Inactive

maxgds approved these changes Dec 12, 2017

View reviewed changes

steventux force-pushed the update-taxonomy-sidebar-design branch from fa16a21 to fe07f25 Compare December 12, 2017 16:07

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 16:07 Inactive

steventux force-pushed the update-taxonomy-sidebar-design branch from fe07f25 to 0f6ea9b Compare December 12, 2017 16:22

fofr temporarily deployed to govuk-static-pr-1222 December 12, 2017 16:22 Inactive

steventux force-pushed the update-taxonomy-sidebar-design branch from 0f6ea9b to e1dc506 Compare December 12, 2017 16:42

fofr deployed to govuk-static-pr-1222 December 12, 2017 16:43 View deployment

NickColley reviewed Dec 15, 2017

View reviewed changes

steventux force-pushed the update-taxonomy-sidebar-design branch from e1dc506 to b5a9524 Compare December 15, 2017 12:34

Replace element selectors with BEM taxonomy styles

dd42118

steventux force-pushed the update-taxonomy-sidebar-design branch from b5a9524 to dd42118 Compare December 15, 2017 12:42

steventux merged commit 87a6746 into master Dec 15, 2017

steventux deleted the update-taxonomy-sidebar-design branch December 15, 2017 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update taxonomy sidebar design #1222

Update taxonomy sidebar design #1222

steventux commented Dec 11, 2017 •

edited

Loading

maxgds left a comment

maxgds Dec 12, 2017

NickColley Dec 12, 2017

NickColley Dec 12, 2017

maxgds Dec 12, 2017

maxgds Dec 12, 2017

maxgds Dec 12, 2017

steventux commented Dec 12, 2017 •

edited

Loading

maxgds commented Dec 12, 2017

NickColley left a comment

maxgds commented Dec 12, 2017

steventux commented Dec 12, 2017 •

edited

Loading

NickColley commented Dec 12, 2017

steventux commented Dec 12, 2017

steventux commented Dec 12, 2017 •

edited

Loading

maxgds left a comment

steventux commented Dec 13, 2017

NickColley left a comment

NickColley Dec 15, 2017

steventux commented Dec 15, 2017

Update taxonomy sidebar design #1222

Update taxonomy sidebar design #1222

Conversation

steventux commented Dec 11, 2017 • edited Loading

maxgds left a comment

Choose a reason for hiding this comment

maxgds Dec 12, 2017

Choose a reason for hiding this comment

NickColley Dec 12, 2017

Choose a reason for hiding this comment

NickColley Dec 12, 2017

Choose a reason for hiding this comment

maxgds Dec 12, 2017

Choose a reason for hiding this comment

maxgds Dec 12, 2017

Choose a reason for hiding this comment

maxgds Dec 12, 2017

Choose a reason for hiding this comment

steventux commented Dec 12, 2017 • edited Loading

maxgds commented Dec 12, 2017

NickColley left a comment

Choose a reason for hiding this comment

maxgds commented Dec 12, 2017

steventux commented Dec 12, 2017 • edited Loading

NickColley commented Dec 12, 2017

steventux commented Dec 12, 2017

steventux commented Dec 12, 2017 • edited Loading

maxgds left a comment

Choose a reason for hiding this comment

steventux commented Dec 13, 2017

NickColley left a comment

Choose a reason for hiding this comment

NickColley Dec 15, 2017

Choose a reason for hiding this comment

steventux commented Dec 15, 2017

steventux commented Dec 11, 2017 •

edited

Loading

steventux commented Dec 12, 2017 •

edited

Loading

steventux commented Dec 12, 2017 •

edited

Loading

steventux commented Dec 12, 2017 •

edited

Loading