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

Taxonomy navigation collections #1008

Merged
merged 2 commits into from
Jul 31, 2018
Merged

Conversation

andrewgarner
Copy link
Contributor

@andrewgarner andrewgarner commented Jul 31, 2018

Moves related collections from the sidebar to the taxonomy navigation.

Before:

screen shot 2018-07-31 at 14 48 38

After:

screen shot 2018-07-31 at 14 49 01

https://trello.com/c/zUWqJgX9


Visual regression results:
https://government-frontend-pr-1008.surge.sh/gallery.html

Component guide for this PR:
https://government-frontend-pr-1008.herokuapp.com/component-guide

No longer display related collections in the sidebar if the new
navigation is enabled.
@benthorner benthorner temporarily deployed to government-frontend-pr-1008 July 31, 2018 13:51 Inactive
.taxonomy-navigation__section:first-of-type {
margin-top: $gutter * 1.5;
.taxonomy-navigation__row {
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this needs to use flex rather than just the grid classes (grid-row and column-one-third)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that they line up with the highlight boxes below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can still line up without flex? Quick testing in the herokuapp looks ok:

screen shot 2018-07-31 at 15 06 37

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The breakpoints are different:

Grid:

ezgif com-video-to-gif
Flex:

ezgif com-video-to-gif-2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, I didn't spot that - that's annoying! Thanks for explaining.

I wonder if we need it to match up or if we could just have each column half the width of that row. But using flex isn't a big deal, it just adds a lot of CSS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was something @miaallers mentioned when we went through it together. I should imagine we can find a better way to implement if the AB test is positive.

Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very small suggestions for improvement, but don't need to be blocking

@include bold-24;
display: block;
.taxonomy-navigation__list-item {
margin-bottom: $gutter / 6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point it might just be more useful to write it out as 5px or perhaps $gutter-one-third / 2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for padding-bottom below

<div class="taxonomy-navigation__row">
<div class="taxonomy-navigation__column">
<%= render "govuk_publishing_components/components/heading",
text: 'Topics',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make this a translated string

<% if related_collections.any? %>
<div class="taxonomy-navigation__column">
<%= render "govuk_publishing_components/components/heading",
text: 'Collections',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to add this as a translated string

@sihugh sihugh merged commit 82e1a02 into master Jul 31, 2018
@sihugh sihugh deleted the taxonomy-navigation-collections branch July 31, 2018 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants