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

Link UI: Fix focus styles w/ suggestions showing #18292

Merged
merged 2 commits into from Nov 7, 2019
Merged

Conversation

@obenland
Copy link
Member

obenland commented Nov 5, 2019

Description

Limits the height of the suggestion list's top & bottom gradient.

How has this been tested?

  • Checked out #18062 and cherry picked this commit.
  • Created a new menu item and started a search for "page" to get a list of suggestions.

Screenshots

Before:

Screen Shot 2019-11-05 at 2 02 28 PM

After:

Screen Shot 2019-11-05 at 2 02 55 PM

Types of changes

Bug fix (non-breaking change which fixes an issue) as described in #18135.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@obenland obenland requested a review from getdave Nov 5, 2019
@obenland obenland self-assigned this Nov 5, 2019
@obenland obenland added this to 👀 PRs to review in Navigation block via automation Nov 5, 2019
@obenland

This comment has been minimized.

Copy link
Member Author

obenland commented Nov 5, 2019

@getdave Do we need those top & bottom gradients at all? They're going from white to transparent on a white background.

Edit: never mind, I see now that they're there to gracefully hint at more results.

@getdave getdave mentioned this pull request Nov 5, 2019
3 of 4 tasks complete
@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 6, 2019

Testing this now...

Also I rebased.

I see now that they're there to gracefully hint at more results.

Yep that's why I added them. Tiny hint, but makes a big different in my manual testing.

@getdave getdave force-pushed the fix/bottom-border branch from 0b8a68c to b952fdf Nov 6, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 6, 2019

LGTM 👍, you will need to rebase it, though.

Copy link
Contributor

retrofox left a comment

:shipit: 👍

Copy link
Contributor

getdave left a comment

When testing this manually, if I change the bottom gradient to red then I can still see it overlapping when scrolling right to the bottom.

Screen Shot 2019-11-06 at 15 19 59

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 6, 2019

A more complex way of doing this might be to add a scroll event listener and check to see if the container has been fully scrolled. If so you can add a class to remove the border.

element.scrollHeight - element.scrollTop === element.clientHeight

However, if we can solve via CSS that would be a lot easier.

@obenland

This comment has been minimized.

Copy link
Member Author

obenland commented Nov 6, 2019

For my understanding, you want the gradient and the last item not to overlap at all? Because currently the background and gradient are both white and you won't notice it until the text gets scrolled down. See also #18135 (comment)

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 6, 2019

My understanding was

  • When the container is already scrolled to the bottom (no more items) then there should be no bottom gradient because there is nothing to hint at.
  • When the container is already scrolled to the top (no more items) then there should be no top gradient because there is nothing to hint at.

The key is that the gradient only shows in the direction where there are items below the visible area.

@obenland

This comment has been minimized.

Copy link
Member Author

obenland commented Nov 6, 2019

Yeah I'm not sure we're doing ourselves a favor by trying to make that dynamic. Let me adjust the gradient first and see if that fixes it enough for now.

@obenland

This comment has been minimized.

Copy link
Member Author

obenland commented Nov 6, 2019

After e0bb753:

Last item, scrolled all the way to the bottom:

Screen Shot 2019-11-06 at 3 11 21 PM

Highlighted item with room to scroll:

Screen Shot 2019-11-06 at 3 11 38 PM

@getdave
getdave approved these changes Nov 7, 2019
@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 7, 2019

I tested this and I can longer see the gradient overlapping. :shipit:

@obenland obenland merged commit 62b9bc2 into master Nov 7, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 7, 2019
@obenland obenland deleted the fix/bottom-border branch Nov 7, 2019
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
* Limit gradient height

* Align bottom gradient with bottom padding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.