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.

Sign up for GitHub

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

Jump to bottom

Nav sidebar scrolls if the items don't fit on the screen #43906

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Nav sidebar scrolls if the items don't fit on the screen #43906

merged 2 commits into from
Jul 10, 2020

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Jul 6, 2020 •
edited
Loading

Changes proposed in this Pull Request

  • Adds a scrollbar to the nav sidebar when it overflows
  • Stick the "create post" and "view all posts" button to the bottom of the sidebar (matches new mockup)

Jul-06-2020 20-20-32

Testing instructions

  • Apply 45973-code to sandbox
  • Shrink browser window
  • It should scroll, should probably check what the default scrollbar looks like on Windows 🤔

@p-jackson p-jackson added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Editor Improvements labels Jul 6, 2020
@p-jackson p-jackson requested a review from a team July 6, 2020 08:21
@p-jackson p-jackson self-assigned this Jul 6, 2020
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=add/nav-bar-scrollbar

@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D45973-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@ramonjd
Copy link
Member

ramonjd commented Jul 7, 2020

Tests well in Chrome and Firefox

The scrollbar looks good in IE too, however the icon disappears when the sidebar is open

Jul-07-2020 14-41-51

@p-jackson p-jackson added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 7, 2020
@p-jackson
Copy link
Member Author

Moving out of review column, because I think this would be a good PR to also implement buttons sticking to the bottom of the sidebar.

@p-jackson p-jackson mentioned this pull request Jul 7, 2020
Closed
@p-jackson p-jackson added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 8, 2020
@p-jackson
Copy link
Member Author

Buttons stuck to the bottom 👍

@p-jackson
Copy link
Member Author

Rebase. Maybe fix the tests?

@p-jackson p-jackson force-pushed the add/nav-bar-scrollbar branch from f5a4bd2 to 10ee188 Compare July 9, 2020 02:44
ramonjd
ramonjd approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Buttons and scroll look good! Works in IE Edge perfectly

Jul-10-2020 10-50-03

My last comment about IE, I should have mentioned, was IE11 and was about the logo. Probably best for another PR since it doesn't deal with buttons or scrollbars 😄

@p-jackson p-jackson merged commit 65157db into master Jul 10, 2020
@p-jackson p-jackson deleted the add/nav-bar-scrollbar branch July 10, 2020 07:39
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 10, 2020
@p-jackson p-jackson mentioned this pull request Jul 10, 2020
Closed
@deBhal deBhal mentioned this pull request Jul 13, 2020
Merged
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramonjd ramonjd ramonjd approved these changes

Assignees

@p-jackson p-jackson

Labels
[Goal] Editor Improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@p-jackson @matticbot @ramonjd