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

Migrating to baron scrollbar WIP #11456

Merged
merged 35 commits into from Apr 12, 2018

Conversation

Projects
None yet
5 participants
@alexanderzobnin
Contributor

alexanderzobnin commented Apr 2, 2018

Trying to solve #11053 by migrating to baron scrollbar which doesn't affect scroll behaviour and only changes its styles.

Work in progress, need more tests on different browsers/OSs/platforms.

@alexanderzobnin alexanderzobnin requested review from torkelo and daniellee Apr 2, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Apr 2, 2018

Codecov Report

Merging #11456 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11456      +/-   ##
==========================================
+ Coverage    51.9%    51.9%   +<.01%     
==========================================
  Files         359      359              
  Lines       26068    26069       +1     
  Branches     1571     1571              
==========================================
+ Hits        13530    13531       +1     
  Misses      11796    11796              
  Partials      742      742

@daniellee daniellee self-assigned this Apr 3, 2018

@daniellee

This comment has been minimized.

Member

daniellee commented Apr 4, 2018

The menu seems to mostly work and scroll position seems fine on Firefox and iOS.

I found a couple of things to fix:

  • Baron is not added to yarn.lock. Needs to be added with yarn add baron.

  • In Chrome and Firefox, the width of the dashboard seems to be incorrect - too wide. Getting horizontal scroll - scrolled to the left:

    image

    scrolled to the right:

    image

  • Getting the error: legendScrollbar.destroy is not a function on small screens when a clicking a link in the mobile menu. When testing on iOS - sometimes getting an empty black screen when clicking on a link in the menu.

@alexanderzobnin

This comment has been minimized.

Contributor

alexanderzobnin commented Apr 4, 2018

@daniellee Thanks for review, I also noticed these issues so working on fixes.

@daniellee

This comment has been minimized.

Member

daniellee commented Apr 4, 2018

@alexanderzobnin found two issues. There is no scroll on the search in the mobile view:

image

The other issue is that on iOS, sometimes after choosing a page from the menu, it does not load. Trying to find out why. It does not happen on the master branch.

@daniellee

This comment has been minimized.

Member

daniellee commented Apr 4, 2018

image

Can reproduce this in Chrome sometimes, though not as often as on an iPhone. On the grafana-scrollbar div, sometimes the max-width and width get set to 0 which hides all the content.

@marefr

This comment has been minimized.

Member

marefr commented Apr 4, 2018

First general concern/question. Is it only my that have problems in Chrome that I can't scroll sometimes, but clicking on some area (giving element focus I suppose) allow me to scroll again. This is really irritating. Maybe my laptop's scroll having issues?

Second general concern: Seems like my homepage dashboard no longer can render starred/recent dashboards and installed apps panels. Possibly no height?

Confirmed that this makes #11185 almost completely solved, at least when using an extension called FireShot in Chrome which basically scrolling the page down. Think this is how most of these extensions worls. Partially because there's still an issue when the sidemenu are visible (see first screenshot), but works very good when in kiosk mode (see second screenshot). Maybe not an issue with sidemenu, it's of course because the top header are fixed.

fireshot capture 22 - grafana - big dashboard_ - http___localhost_3000_d_000000003_big-dashboard

fireshot capture 16 - grafana - big dashboard_ - http___localhost_3000_d_000000003_big-dashboard

@@ -31,13 +60,25 @@ export function geminiScrollbar() {
scope
);
// force updating dashboard width
appEvents.on('toggle-sidemenu', forceUpdate);

This comment has been minimized.

@torkelo

torkelo Apr 5, 2018

Member

this could be a memory leak, no unsubscribe (or scope passed as last argument to unsubscribe on scope destory)

This comment has been minimized.

@alexanderzobnin

alexanderzobnin Apr 5, 2018

Contributor

Ah, yea, missed that.

@torkelo

This comment has been minimized.

Member

torkelo commented Apr 5, 2018

Good work alex!

Tested this and not sure if this is a remaining issue or not but the legend height is buggy (to high).

screen shot 2018-04-05 at 09 07 04

@marefr

This comment has been minimized.

Member

marefr commented Apr 11, 2018

Still things left to fix:

  1. PNG (phantomjs) rendering is not working for graph panel having legends to the right
  2. Starred/recent dashboards and installed apps panel doesn't render correctly

image

@marefr

This comment has been minimized.

Member

marefr commented Apr 11, 2018

We have a new branch now as well called 11053-remove-native-scrollbar where we've added the native scrollbar to all pages.

marefr and others added some commits Apr 11, 2018

panel: add baron scroller to correct element
This resolves issue with alert list panel getting scrollbars
attached to incorrect element. Now the panel content are
rendered correctly and all content are displayed as
expected.
fix so that dash list panel are rendered correctly
This resolves issue with dash list panel getting scrollbars
    attached to incorrect elements. Now the panel content are
    rendered correctly and all content are displayed as
    expected.
fix so that page scrollbars can be scrolled by keyboard on page load
The page scrollbars are custom, not rendered on the body element and
with css property overflow set for scroll to be enabled.
For being able to scroll the page using the keyboard when a
page loads, some custom code was needed.
This fix should both work when doing a full reload of a url and when
navigating to other pages/dashboards.
For those pages having an input field that are focused on load,
scrolling by keyboard (arrow up/down) will obviously not work.
dashboard: show baron scrollbar in dashboard panel when mouse is over
This should make the scrolling user experience much better since it
will highlight and show that content actually can be scrolled when
hovering over a panel.
scrollbar: remove unused div
Fixes the height of the settings menu
minor scrollbar fixes
Hide page scrollbar when rendering using phantomjs.
Hide baron scrollbar when rendering using phantomjs.
scrollbar: fixes continuation scrolling for iOS
The -webkit-overflow-scrolling is an iOS only property:

https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-overflow-scrolling

that turns on momementum/continuation scrolling for iOS devices. This means
that when swiping, the scroll continues for a half second rather than instantly
stopping the scroll when the user lifts their finger from the screen.
scrollbar: fix so no overflow for legend under graph
Adds 1px of padding to the graph legend scroll div so that a non-table
legend does not get an unnecessary scroll bar.
@daniellee

Fantastic work by Alex to get this working. Loads of tricky edge cases (phantomjs, firefox etc.) that needed to be solved.

@daniellee daniellee merged commit b481e15 into grafana:master Apr 12, 2018

4 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-backend Your tests passed on CircleCI!
Details
ci/circleci: test-frontend Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment