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

Reorder the navigation lists vertically #2303

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Reorder the navigation lists vertically #2303

merged 3 commits into from
Sep 15, 2021

Conversation

injms
Copy link
Contributor

@injms injms commented Sep 9, 2021

What

Changes the layout of list of links in the navigation to use CSS grid layout so that it can be alphabetically ordered vertically, rather than horizontally.

Card: https://trello.com/c/BLSqfgij

Why

Lists are easier to scan when each column is alphabetically sorted vertically. This is possible to do automagically with CSS columns, but Chrome doesn't like CSS columns when links are using text-decoration-thickness (Issue on GOV.UK Frontend; Issue on Chromium) - so another solution was needed.

CSS Grid Layout has support going back to IE 10 and allows for vertical columns as long as the number of items in the columns are known - so when the navigation is updated the CSS controlling the grid needs to be updated.

Visual Changes

Before After
image image
image image

Internet Explorer 11:
Screenshot 2021-09-09 at 13 26 31
Screenshot 2021-09-09 at 13 26 19

Internet Explorer 10:
Screenshot 2021-09-09 at 13 27 54
Screenshot 2021-09-09 at 13 28 04

Fallback in Internet Explorer 9:

The list does continue - I wasn't able to capture it in a single screenshot:
Screenshot 2021-09-09 at 13 22 45

Screenshot 2021-09-09 at 13 22 27

@bevanloon bevanloon temporarily deployed to govuk-publis-reorder-na-swwffw September 9, 2021 10:30 Inactive
@injms injms marked this pull request as ready for review September 9, 2021 12:29
@kr8n3r
Copy link
Contributor

kr8n3r commented Sep 9, 2021

depends on how you get items back from backend, you could also add a class and do

// items 1-9 e.g
item.column-a {grid-column: 1;}

// items 10-19 e.g
item column-b {grid-column: 2;}

@injms
Copy link
Contributor Author

injms commented Sep 9, 2021

depends on how you get items back from backend, you could also add a class and do

// items 1-9 e.g
item.column-a {grid-column: 1;}

// items 10-19 e.g
item column-b {grid-column: 2;}

I could, but I'd still need to do something with the rows. The previous version of CSS grid that IE support needs the position of the element in the grid set explicitly - both row and column - or it defaults to 1 and then overlaps.

I did attempted to reduce the number of rules by combining them, but it was super-unreadable and not at all easy to debug. Whilst this is verbose, I think it's more understandable.

@owenatgov
Copy link
Contributor

After ticking over this I understand why you've taken this approach, considering the problem. It's better to be clear than clever. But:

If a dev wants to make a change to the content in the new header which at all changes the number of items, they have to make this change in 2 places: the yml files, the big list of nth items and possibly the grid-template-rows and -ms-grid-rows. This creates a risk of a dev forgetting to make the scss changes, especially if they're in a rush, and relies on an in-the-know FE (aka you or me right now) to hop in and remind them to update in the scss. This, in my opinion, defeats the purpose of the added verbosity as the code for the new header component as a whole is no longer self documenting.

Could we pop in a test somewhere to try and catch these numbers? I'm not totally sure what this would look like, I guess it'd be a js test to better interrogate styling and would probs be quite complex. My thinking is that it would stop devs forgetting to at least check the scss file and update it.

@injms
Copy link
Contributor Author

injms commented Sep 14, 2021

@owenatgov I've scratched my head a bit and can't find a way of testing the number of items in the localisation files against the CSS being output - so I've added a SCSS mixin to make updating the grid easier and a couple of comments in the YAML to nudge people that there needs to be extra work done. This isn't perfect, but it means that when an item is added or taken away it's a lot less work and a lot simpler.

If only CSS columns worked with text-decoration-* then none of this would be needed. Ah well.

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

One final nitpick but this is basically ready to roll. I see what you mean about it being a bit knotty but I still think it's a pretty robust solution to a tricky problem. I agree it's a shame that column-layout doesn't just work properly.

Lists are easier to scan when each column is alphabetically sorted vertically.
This is possible to do automagically with CSS columns, but Chrome doesn't like
CSS columns when links are using `text-decoration-thickness` [1][2] - so another
solution was needed.

CSS Grid Layout has support going back to IE 10[3] and allows for vertical
columns as long as the number of items in the columns are known - so when the
navigation is updated the CSS controlling the grid needs to be updated.

[1]: alphagov/govuk-frontend#2204
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1190987
[3]: https://caniuse.com/css-grid
The mixin outputs the styles needed for the grid layout to work in IE10+ - this
is quite a repetative syntax, so the mixin takes the number of items and the
number of columns and works out the rest.

Added comments to the pertinent parts of the navigation localisation to remind
future people that they need to update the styles as well.
@injms injms merged commit 982c438 into master Sep 15, 2021
@injms injms deleted the reorder-nav-lists branch September 15, 2021 08:44
@injms injms mentioned this pull request Sep 15, 2021
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.

4 participants