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

Changes required when there are only one "part of" step by step navs #458

Merged

Conversation

DilwoarH
Copy link
Contributor

@DilwoarH DilwoarH commented Jul 31, 2018

Adds styling for singular related step by step navs

Changes

  • Increase size of font to be the same size as the step by step headings
  • Increase the margin-top for the "part of" text to $gutter-two-third
  • Decrease the margin-bottom for the "part of" text to $gutter-one-quarter
  • Decrease the margin-bottom of step by step title to $gutter-one-third + 3

Notes:

Change number 4 is being done as there is a padding of 7px on the show all button which makes the margin-bottom seem much larger.

This should not affect cases where there are multiple step by steps.

Ticket: https://trello.com/c/zlQ0Go3P/700-fix-the-step-by-step-title-sizing-and-spacing-on-mobile-when-it-appears-as-a-footer-s

Screenshot

screen shot 2018-07-31 at 14 16 11


Component guide for this PR:
https://govuk-publishing-compon-pr-458.herokuapp.com/component-guide/step_by_step_nav_related

@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-458 July 31, 2018 13:22 Inactive
@@ -3,7 +3,9 @@
pretitle ||= t("govuk_component.step_by_step_nav_related.part_of", default: "Part of")
%>
<% if links.any? %>
<div class="gem-c-step-nav-related" data-module="track-click">
<div
class="gem-c-step-nav-related <% if links.length == 1 %> gem-c-step-nav-related-singular <% end %>"
Copy link
Contributor

@andysellick andysellick Aug 1, 2018

Choose a reason for hiding this comment

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

Could be written more cleanly as

class="gem-c-step-nav-related <%= "gem-c-step-nav-related-singular" if links.length == 1 %>" 

You might even not need the == 1 part, worth trying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I will update this, we'll need one as we only want it to match if length is 1

@@ -15,6 +15,22 @@
padding: 0;
}

.gem-c-step-nav-related-singular {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class name doesn't follow our BEM convention - should be gem-c-step-nav-related--singular.

margin-bottom: $gutter-one-third + 3;
}

.gem-c-step-nav-related-singular .gem-c-step-nav-related__heading {
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 this whole section would be more readable if the classes were nested.

}

.gem-c-step-nav-related-singular .gem-c-step-nav-related__heading {
@include _core-font-generator(19px, 19px, 19px, 1.4, 1.4, false, bold);
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed you use the core-font-generator mixin a lot - are you aware you can use @include core-19 or @include bold-19 instead? (see here). Obviously if you want to use custom line heights etc. then that's fine, although we should use the standard mixins where reasonable, just for consistency and ease of maintenance.

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 reason why I used the core-font-generator is because we needed the related size to be consistent with the step title. https://github.com/alphagov/govuk_publishing_components/blob/master/app/assets/stylesheets/govuk_publishing_components/components/_step-by-step-nav.scss#L62

If I use bold-19 or core-19, the font size variable between mobile and desktop.

- Increase size of font to be the same size as the step by step headings
- Increase the margin-top for the "part of" text to $gutter-two-third
- Decrease the margin-bottom for the "part of" text to $gutter-one-quarter
- Decrease the margin-bottom of step by step title to $gutter-one-third + 3

Change number 4 is being done as there is a padding of 7px on the show all button which makes the margin-bottom seem much larger.

This should not affect cases where there are multiple step by steps.

Ticket: https://trello.com/c/zlQ0Go3P/700-fix-the-step-by-step-title-sizing-and-spacing-on-mobile-when-it-appears-as-a-footer-s
@DilwoarH DilwoarH force-pushed the dh-700-fix-the-step-by-step-title-sizing-and-spacing branch from 51fbef2 to 877700e Compare August 2, 2018 08:09
@benthorner benthorner had a problem deploying to govuk-publishing-compon-pr-458 August 2, 2018 08:10 Failure
@DilwoarH
Copy link
Contributor Author

DilwoarH commented Aug 2, 2018

@andysellick - updated

@DilwoarH DilwoarH merged commit b26c7ba into master Aug 2, 2018
@DilwoarH DilwoarH deleted the dh-700-fix-the-step-by-step-title-sizing-and-spacing branch August 2, 2018 10:35
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.

3 participants