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

left sidebar: Avoid unnecessary scrollbar. #13289

Merged
merged 1 commit into from Oct 30, 2019

Conversation

@davidtwco
Copy link
Contributor

davidtwco commented Oct 15, 2019

This pull request replaces the margin-bottom styling on the left stream sidebar with an empty element with the same height. This preserves the intended behaviour of the commit which introduced the margin, to fix #12519 while removing an unnecessary scrollbar which could hide the top-most stream in the stream list, which resolves #13050.

Testing Plan:
Visually confirmed the desired result while preserving the previous behaviour.

GIFs or Screenshots:
With sufficient vertical space, there is no scrollbar:
image

When there is a scrollbar, there is margin at the bottom which stops the browser's currently-hovered-over url from obscuring the button:
image

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented Oct 15, 2019

Hello @zulip/server-sidebars members, this pull request was labeled with the "area: left-sidebar" label, so you may want to check it out!

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Oct 15, 2019

I experimented for a while trying to find a css rule that I could modify or add which would achieve the desired effect, but adding an empty element was the only solution which worked - I'm open to changing the approach if anyone has a better solution.

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Oct 15, 2019

I'm not sure if the CI failure is spurious, it seems to have failed in the documentation_crawler but I don't think I've modified anything that would affect that?

@mateuszmandera

This comment has been minimized.

Copy link
Collaborator

mateuszmandera commented Oct 15, 2019

I'm getting the same on my PR #13256 so I think this mustbe something unrelated to our changes.

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Oct 15, 2019

Fixed in 0ce5086. Yeah, I'm not sure what caused that link to stop working -- it might be some change on the ReadTheDocs upstream side of things? In any case, definitely not your fault.

@timabbott timabbott force-pushed the davidtwco:issue-13050-scroll-bar branch from ceb5ae0 to 0a7d59b Oct 15, 2019
@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Oct 15, 2019

@andersk are taking a look at this. It seems like what we actually want to do is delete a bunch of code in resize.js and replace it with CSS. We'll see if we can figure out how to make that work.

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Oct 28, 2019

@timabbott @andersk any updates on this? My understanding was that the way that the padding was added (on the .simplebar-content class from simplebar) prior to this PR meant that simplebar's height calculations didn't factor it in. As far as I could tell, resize.js wasn't particularly relevant to the issue - no changes I made to it were able to achieve the desired behaviour.

@andersk

This comment has been minimized.

Copy link
Member

andersk commented Oct 28, 2019

Tim was summarizing a long conversation we had about future directions. But the margin on .simplebar-content (from #12562) was definitely wrong since that’s internal to SimpleBar.

What do you think about turning add-stream-link into a div:

--- a/templates/zerver/app/left_sidebar.html
+++ b/templates/zerver/app/left_sidebar.html
@@ -76,3 +76,5 @@
                 {% if show_add_streams %}
-                <a id="add-stream-link" href="#streams/all"><i class="fa fa-plus-circle" aria-hidden="true"></i>{{ _('Add streams') }}</a>
+                <div id="add-stream-link">
+                    <a href="#streams/all"><i class="fa fa-plus-circle" aria-hidden="true"></i>{{ _('Add streams') }}</a>
+                </div>
                 {% endif %}

and putting the margin on that?

@davidtwco davidtwco force-pushed the davidtwco:issue-13050-scroll-bar branch from 0a7d59b to 9265902 Oct 28, 2019
@davidtwco

This comment has been minimized.

Copy link
Contributor Author

davidtwco commented Oct 28, 2019

@andersk I've updated the PR to take the approach you've suggested in your comment.

This commit modifies the `#add-stream-link` element to be a `div`
containing the previous `a` element. The margin that was added to
`#stream-filters-container .simplebar-content` is then moved to that new
`div`.

This preserves the intended behaviour of the commit which introduced
the margin, to fix #12519 while removing an unnecessary scrollbar
which could hide the top-most stream in the stream list.

Fixes #13050

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the davidtwco:issue-13050-scroll-bar branch from 9265902 to c00e66d Oct 28, 2019
@timabbott timabbott merged commit 7fc72df into zulip:master Oct 30, 2019
4 checks passed
4 checks passed
ci/circleci: bionic-backend-python3.6 Your tests passed on CircleCI!
Details
ci/circleci: xenial-backend-frontend-python3.5 Your tests passed on CircleCI!
Details
codecov/project 91.67% (+0.02%) compared to f077508
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Oct 30, 2019

Merged, thanks folks!

@davidtwco davidtwco deleted the davidtwco:issue-13050-scroll-bar branch Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.