[issue][668] - Data browser automatically scrolls indefinitely

[pivanov]

#668

You can see a video demo of the patch here.

[flovilmart]

Thanks for the PR, can you explain a bit more why the dashboard was autoscrolling and how this PR fixes the issue?

[pivanov]

Sure ... as @JacerOmri said the problem is that:

Actually, the height of the spacer above the actual table shown is changing, and i think this fires the scroll event again.

The problem appears only on chrome for mac os. I don't have time to go deep and check what cause this automatically scroll but anyway there is a performance issue with these "above" and "below" spacers. Another issue is that each row have "min-width" inline style which is not a good idea.

What I did:
Basically I just removed the "above" and "below" spacers. Then I add a wrapper around the rows and set the minWidth on it here.

I've also used a few lines of CSS to set different colors for the "DataBrowserHeaderBar" component because there is no need to calculate "odd" and "even" elements and set the inline style for each of them.

These changes works better for us than the original approach so I hope you will enjoy them :)

[steven-supersolid]

I did a quick review and looks OK with a couple of caveats (disclaimer: I'm not a web developer)

1. It's hard to see the effect just by inspecting code, so ideally it should be tested on various browsers and platforms - i.e. we don't want this change to fix MacOS Chrome but break Windows Firefox. IMHO responsibility for this should be with the author although may need support for other platforms

2. There are a number of changes in this review mentioned above - it is easier and faster to review PR's if they contain fewer changes, just need to make more PR's

So I provisionally approve as I know this is a useful bug fix but would welcome more eyes on this and smaller reviews in future :)

[pivanov]

Hey @steven-supersolid Thanks for your feedback.
I agree that is hard to review this if you don't look this code everyday (like me :))

1. These changes works perfect in our (SashiDo.io) Dashboard for several months.
2. Yeah I can split the PR to few others if you want (just let me know).

[steven-supersolid]

Hi @pivanov, we had a discussion among some of the maintainers and would like to get to the root cause of the issue before fixing it, as it is thought that it was introduced recently.

Would you mind splitting the review so each part can be approved separately, and if you have time to identify when the bug was introduced and ensure your new PR for this addresses it specifically? If the bug was always there then that is fine but would be useful to know either way.

We also have another PR open for the same bug so would be worth checking that out too: #671

Sorry that this is a lot for what seems a simple bug!

[facebook-github-bot]

@pivanov updated the pull request - view changes

[pivanov]

The bug was introduced on Chrome v56.0.2924 for Mac OS X and I think is related to some rendering changes (don't have time to goo deep to this).

The PR #671 partially fix the issue. I had a chance to test it and it's not working well.

@JacerOmri: I was able to make it stop, it feels a bit laggy, but at least it's usable.

I will open another PR for the rest of my changes if we merge this one.

[facebook-github-bot]

@pivanov updated the pull request - view changes

[facebook-github-bot]

@pivanov updated the pull request - view changes

[TylerBrock]

Would it be helpful if we figured out which commit created the bug in the first place using git-bisect?

[jeacott1]

not just an osx issue. this is a problem in chrome on windows too.

[jeacott1]

This PR seems to do wacky things to the rhs scroll bar.
as I scroll down, the scrollbar handle shrinks as you'd expect (jumps around a bit though), but as you scroll back up the handle grows again in a very glitchy way.
has anyone else noticed this?
I also cant properly 'grab' the rhs scroll handle. it is impossible to drag the scrollbar.

chrome. edge. windows 10. also osx chrome, safari, probably everything else.

[mortizbey]

Hi, any ETA on the merging of this PR?

[jeacott1]

@ortimanu do you think it works well enough? does the scrollbar work properly for you?

[mortizbey]

@jeacott1 nop. Same thing happens to me.

[pivanov]

Hey all,
As I already mention:

These changes works perfect in our (SashiDo.io) Dashboard for several months.

I will update the PR these days if you still want to merge/get the changes.

[mortizbey]

Ok, maybe I am doing something wrong. Will recheck when merged.

[mortizbey]

What is the ETA for merging this to master?

[flovilmart]

@pivanov I'm coming back to that PR, can you please split it up with the fix on one and the other additions on the other? You seem to have added a bunch of additional changes unrelated with the fix, and I'd like to review them separately, ideally with screenshots of before / after. Closing this one for now.