-
Notifications
You must be signed in to change notification settings - Fork 327
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
Fix whitespace affecting text alignment in pagination block variant #5066
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 055c550 |
Stylesheets changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index 7c385603d..801fafa64 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -4495,8 +4495,7 @@ only screen and (min-resolution:2dppx) {
text-decoration: underline;
text-decoration-thickness: max(1px, .0625rem);
text-underline-offset: .1578em;
- display: inline-block;
- padding-left: 30px
+ display: inline-block
}
.govuk-pagination__icon {
@@ -4519,59 +4518,43 @@ only screen and (min-resolution:2dppx) {
display: block
}
-.govuk-pagination--block .govuk-pagination__item {
- padding: 15px;
- float: none
-}
-
.govuk-pagination--block .govuk-pagination__next,
.govuk-pagination--block .govuk-pagination__prev {
padding-left: 0;
float: none
}
-.govuk-pagination--block .govuk-pagination__next {
- padding-right: 15px
+.govuk-pagination--block .govuk-pagination__next .govuk-pagination__link,
+.govuk-pagination--block .govuk-pagination__prev .govuk-pagination__link {
+ display: inline-block
}
-.govuk-pagination--block .govuk-pagination__next .govuk-pagination__icon {
- margin-left: 0
+.govuk-pagination--block .govuk-pagination__next {
+ padding-right: 15px
}
.govuk-pagination--block .govuk-pagination__prev+.govuk-pagination__next {
border-top: 1px solid #b1b4b6
}
-.govuk-pagination--block .govuk-pagination__link,
-.govuk-pagination--block .govuk-pagination__link-title {
- display: inline
-}
-
.govuk-pagination--block .govuk-pagination__link-title:after {
content: "";
display: block
}
.govuk-pagination--block .govuk-pagination__link {
+ padding-left: 30px;
text-align: left
}
-.govuk-pagination--block .govuk-pagination__link:focus .govuk-pagination__link-label {
- outline: 3px solid transparent;
- color: #0b0c0c;
- background-color: #fd0;
- box-shadow: 0 -2px #fd0, 0 4px #0b0c0c;
- text-decoration: none;
- -webkit-box-decoration-break: clone;
- box-decoration-break: clone
-}
-
.govuk-pagination--block .govuk-pagination__link:not(:focus) {
text-decoration: none
}
.govuk-pagination--block .govuk-pagination__icon {
- margin-right: 10px
+ margin-top: .326em;
+ margin-left: -30px;
+ float: left
}
.govuk-panel {
Action run for 055c550 |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/components/pagination/_index.scss b/packages/govuk-frontend/dist/govuk/components/pagination/_index.scss
index 9bd247007..7ae869ac4 100644
--- a/packages/govuk-frontend/dist/govuk/components/pagination/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/pagination/_index.scss
@@ -151,7 +151,6 @@
@include govuk-typography-weight-regular;
@include govuk-link-decoration;
display: inline-block;
- padding-left: govuk-spacing(6);
}
.govuk-pagination__icon {
@@ -175,23 +174,18 @@
.govuk-pagination--block {
display: block;
- .govuk-pagination__item {
- padding: govuk-spacing(3);
- float: none;
- }
-
.govuk-pagination__next,
.govuk-pagination__prev {
padding-left: 0;
float: none;
+
+ .govuk-pagination__link {
+ display: inline-block;
+ }
}
.govuk-pagination__next {
padding-right: govuk-spacing(3);
-
- .govuk-pagination__icon {
- margin-left: 0;
- }
}
// Only apply a border between prev and next if both are present
@@ -199,13 +193,6 @@
border-top: 1px solid $govuk-border-colour;
}
- // Reset both these elements to their inline default, both to ensure that
- // the focus state for block mode "shrink wraps" text as expected
- .govuk-pagination__link,
- .govuk-pagination__link-title {
- display: inline;
- }
-
// Set the after pseudo element to a block which makes the title visually
// display as block level whilst programmatically being inline. We do this
// to get around an NVDA quirk where adjacent block level elements are
@@ -216,24 +203,24 @@
}
.govuk-pagination__link {
+ padding-left: govuk-spacing(6);
text-align: left;
- &:focus {
- // Apply focus styling to the label within the link as if it were being
- // focused to get around a display issue with a focusable inline element
- // containing a mixture of inline and inline-block level elements
- .govuk-pagination__link-label {
- @include govuk-focused-text;
- }
- }
-
&:not(:focus) {
text-decoration: none;
}
}
.govuk-pagination__icon {
- margin-right: govuk-spacing(2);
+ // This magic number is brought to you by the following equation:
+ // ((lineHeight − arrowHeight) ÷ 2) ÷ fontSize
+ // ((25 − 13) ÷ 2) ÷ 19 = 0.326em
+ //
+ // This could have been done programmatically but we don't have functions
+ // for grabbing the line-height of specific typography sizes just yet.
+ margin-top: 0.326em;
+ margin-left: govuk-spacing(6) * -1;
+ float: left;
}
}
}
Action run for 055c550 |
3c61b63
to
dca5a10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is looking good.
If memory serves, all of these dodgy attributes were in service of trying to shrink wrap our focus state. Given that it appears to be causing problems elsewhere, I'm perfectly happy to drop it.
I'm gonna pause at approving as I think it's worth getting a designer to cast one final eye over the focus state change to verify that it's ok.
A final thought is that I wonder if there's an opportunity here to refactor pagination, get rid of the annoying floats etc. I'm very aware that you started this but it didn't get lots of traction. Maybe it's something we can revisit and hopefully actually get done 🤞🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visually it looks much better!
That refactor was more concerned with altering the functionality, in part to try and reduce complexity regarding the behaviour of ellipses and options hiding on mobile should behave. It's fairly standalone from what is happening in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Percy approved as well 👍🏻
dca5a10
to
055c550
Compare
An attempt at fixing the issue where whitespace (or the lack of it) changes the alignment of the title text in the Pagination component's 'block' variant, as described in #3554.
Changes
inline-block
, allowing for the addition ofpadding-left
that affects all text within the component.margin-left
—preventing them from directly influencing text flow calculations.Thoughts
Focus style difference
This PR does slightly change the focus state on these links, with the focus state now stretching to fit the longest line of text, rather than separately 'shrinkwrapping' the title and label lines.
I was unable to find a simple way around this using only CSS and wanted to avoid a breaking HTML change.
I considered removing the focus style from the actual link and re-applying it to the two child elements, but doing so introduced more complexity than I'd like, so I'm gonna 🤞 and hope this is okay until we're content with making a breaking change.
0.326 is the magic number
This also introduces a magic number, the
margin-top
on the arrows themselves. Being floated means they no longer automatically align with the text baseline, so some manual offsetting was required.This could potentially be worked out programmatically: it's$(lineHeight-arrowHeight)\div2$ , specifically $(25-13)\div2=6px$ with numbers added, which is ≅0.326em in this context.
However, we don't presently have a function for getting just the
line-height
for a specific type size and breakpoint, and I was still trying to avoid adding unnecessary complexity, and the pagination'sline-height
doesn't change per breakpoint anyway, so I've hardcoded it. This value is precise enough to visually match the arrow's previous position on even high zoom levels.These comparison screenshots were made in Firefox at 300% zoom on a 72 DPI display.