Skip to content
This repository has been archived by the owner on Dec 2, 2020. It is now read-only.

TwentyTwentyOne: Add bottom-border to hover states in pagination links #875

Merged
merged 6 commits into from
Dec 1, 2020

Conversation

allancole
Copy link
Collaborator

Fixes: #868

Summary

This PR simply adds bottom-borders to the hover state on pagination links to make them match the current page bottom-border.

Test instructions

This PR can be tested by following these steps:

  1. Visit any post archive page (search, category, etc.)
  2. Scroll to the bottom and hover over the pagination links.
  3. You should see a thin underline beneath the hovered link that matches the bottom-border for the current page text. (see screenshots below)

Screenshots

Before:
2020-11-23 16 32 03

After:
2020-11-23 16 30 25

Quality assurance

  • I have thought about any security implications this code might add.
  • I have checked that this code doesn't impact performance (greatly).
  • I have tested this code to the best of my abilities
  • I have checked that this code does not affect the accessibility negatively

@melchoyce
Copy link
Contributor

LGTM 👍

@carolinan
Copy link
Contributor

carolinan commented Nov 23, 2020

I don't think these borders would match any other link hover styles? I don't think it looks bad but it will be inconsistent?

@melchoyce
Copy link
Contributor

Oh, you're right — I had them reversed in my head 😵 💫 Ideally the post navigation styles would match the menu styles: solid action, dotted hover:

image

@allancole
Copy link
Collaborator Author

Ahhh, I didn’t catch that! I’ll reverse this so it matches the menu styles.

@allancole
Copy link
Collaborator Author

allancole commented Nov 24, 2020

Should be good to go now @melchoyce & @carolinan:

Header Nav:
2020-11-24 11 28 32

Pagination:
2020-11-24 11 27 14

@allancole
Copy link
Collaborator Author

Also just noticed the font-sizes don’t match between the header navigation and pagination links:

20px header nav 24px pagination
image image

Since the hover actions are the same, should the font-sizes also the same @melchoyce ?

@melchoyce
Copy link
Contributor

I think I want to leave them as-is. Thanks for checking!

@ryelle
Copy link
Collaborator

ryelle commented Nov 24, 2020

I noticed that the current item has the hover state too, but it's not a link. That could be confusing, I'm not sure if it's technically an a11y issue or not. Could .current also be excluded from the hover state?

@carolinan
Copy link
Contributor

carolinan commented Nov 25, 2020

Just a note that with this change we have solid underlines for next and previous posts (single view), but dotted underlines for the newer/older post pagination. @melchoyce Is that what you want?

@allancole
Copy link
Collaborator Author

Could .current also be excluded from the hover state?

Ahh, good call. I’ll correct that :-)

- Adds dotted underline to singlar next/prev links
- Removes hover state from current page number since it isn’t a link
@allancole
Copy link
Collaborator Author

Just a note that with this change we have solid underlines for next and previous posts (single view), but dotted underlines for the newer/older post pagination. @melchoyce Is that what you want?

I went ahead and corrected this in that last batch of commits. Feels more consistent this way but happy to change it back if you prefer, @melchoyce. Cc: @carolinan

@melchoyce
Copy link
Contributor

Can you add another gif for comparison @allancole?

@allancole
Copy link
Collaborator Author

No prob, @melchoyce. See below:

Archive View

Before:
2020-11-30 16 24 40

After:
2020-11-30 16 25 25

Singular View

Before:
2020-11-30 16 23 30

After:
2020-11-30 16 18 28

@pattonwebz
Copy link
Member

The after gifs look real nice to me ❤️

@melchoyce
Copy link
Contributor

Same, thanks @allancole 👍

@allancole
Copy link
Collaborator Author

I think this is good to merge then, @carolinan—if you’d like to do the honors ;-)

@carolinan
Copy link
Contributor

I am very confused about why one of the after gifs have both a dotted and solid underline?

@carolinan carolinan merged commit 64275b9 into trunk Dec 1, 2020
@carolinan carolinan deleted the fix/issue-868 branch December 1, 2020 19:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post pagination current item style
5 participants