Spacing of tabs list updated to be more inline with similar lists on GOV.UK and the Design System #1330
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1330
hannalaakso
changed the title
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1330
36degrees
approved these changes
May 10, 2019
There was a problem hiding this comment.
Thanks for splitting this out 👍
I wonder how relevant the fact that this 'makes it consistent with the Design System' actually is – I suspect aligning the two wasn't actually our motivation, it's just that the spacing in the navigation in the Design System worked better so we adopted the same spacing here too?
I'm not sure people reading the changelog would understand why consistency with styles in the Design System is important, and the comment feels like it's likely to go out of date.
Thoughts?
|
Spacing of list updated to be more inline with similar lists on GOV.UK and the Design System. GOV.UK Example GOV.UK Design System example |
hannalaakso
changed the title
hannalaakso
changed the title
hannalaakso
force-pushed
the
update-tabs-spacing
branch
from
4b655e4 to
e7ad87f
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1330
36degrees
reviewed
May 10, 2019
src/components/tabs/_tabs.scss
Outdated
| padding-top: govuk-spacing(2); | ||
| padding-bottom: govuk-spacing(2); | ||
| // Use bottom margin on mobile and when JS is not enabled. | ||
| // Consistent with what we do with on-page navigation in GOV.UK Design |
There was a problem hiding this comment.
I think this comment can probably just be removed? I don't think we'd have written it if we'd built the component this way from scratch, and I think it's likely just to go out of date.
hannalaakso
force-pushed
the
update-tabs-spacing
branch
from
e7ad87f to
6b8adff
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1330
Split from #1326 This amends the tab headings spacing on mobile and no-js view to be consistent with how we present the navigation on the Design System: Adjusts the padding/margin so that with the the list version of component that’s used on mobile and no-js we use have govuk-spacing(2) as bottom margin, consistent with what we do with on-page navigation in GOV.UK Design System when it's displayed as a list. For the desktop/js-enabled version of component remove bottom margin and use top and bottom padding instead. Also adjusts bottom margin of list item so that it’s applied on no-js version and add some more spacing under the tabs title.
hannalaakso
force-pushed
the
update-tabs-spacing
branch
from
6b8adff to
956aba6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #1330 (comment)
This PR:
govuk-spacing(2)as bottom margin. For the desktop/js-enabled version of component remove bottom margin and use top and bottom padding instead.Split from #1326