Ability to provide custom scrollbars: #507

rtrg · 2019-12-10T14:20:17Z

Description

This PR provides ability to pass custom scrollbars to FDT.
To disable fdt default scrollbars, need to pass defaultScrollbars prop as false
Can pass custom scrollbars by passing scrollbarX, scrollbarY.
To update custom scrollbars state or trigger fdt scroll, pass onScrollBarsUpdate prop (function) which gets necessary arguments.

How Has This Been Tested?

Made sure all test cases are passing, by updating necessary tests.
Tested manually in examples that scrolling works.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

wcjordan

Thanks for the PR! This looks great, and inline with were we'd like to go. I added a few suggestions, but nothing major / blocking.

I also noticed that the Reorderable Columns example seems broken on this branch and doesn't show the unfrozen columns. Could you take a look and see what the issue might be? Thanks.

wcjordan · 2019-12-16T20:50:42Z

src/FixedDataTable.js

@@ -467,6 +493,9 @@ class FixedDataTable extends React.Component {
    keyboardPageEnabled: false,
    touchScrollEnabled: false,
    stopScrollPropagation: false,
+    defaultScrollbars: true, // For documentation purpose only


Was React complaining about these missing here? If not, I recommend we remove from here, and then update the PropTypes above with a comment like

TODO (jordan) Remove propType of defaultScrollbars without losing documentation (handled in FDTContainer.js)

That way it will be similar to other cases we have.

intention was to show default values in documentation, I made suggested changes.

wcjordan · 2019-12-16T20:51:10Z

src/FixedDataTable.js

@@ -613,6 +642,10 @@ class FixedDataTable extends React.Component {
    this._contentHeight = contentHeight;
  }

+  shouldComponentUpdate(nextProps) {


Was this necessary to fix issues, or just a nice perf enhancement here?

not specific to this PR, in general good to have.

FixedDataTable has a few objects being passed as props. So I suggest removing the shouldComponentUpdate check here, especially since it's not required by the PR.

If we are absolutely certain that the passed in objects are immutable and that they won't change in the future, then we could have this in.

wcjordan · 2019-12-16T20:52:46Z

src/FixedDataTable.js

+          scrollToY: this._scrollToY
+      };
+      if (!shallowEqual(this.previousScrollState, newScrollState)) {
+          this.props.onScrollBarsUpdate(newScrollState);


This is a lot to report to the callback here. Is all of this information necessary, or could we trim it down?

I'm concerned if we expose too many internal details, we won't be able to modify the API easily in the future. Not an issue if we do need to expose all this, but if there are any ways we could slim the API down, it's worth investigating.

I believe we have to pass visbileRowsHeight, allRowsHeight to determine scrollbarY height and its thumb height. scrollY to position scrollbarY thumb. scrollbarYOffsetTop to position scrollbarY itself. Similarly for scrollbarX

We can avoid scrollToX and scrollToY as they can be achieved via scrollTo, but I added them as browser native scrollbar has those methods. Please let me know your opinion here if its ok to have only scrollTo

Okay sounds good. I think passing both scrollToX and scrollToY is fine.

I wish we didn't need to pass scrollbarXOffsetTop & scrollbarYOffsetTop... But understandable if there's no other way.

We also might want to consider renaming visibleRowsHeight & allRowsHeight. Maybe to viewportHeight & contentHeight? Likewise for columns / width.

We also might want to consider renaming visibleRowsHeight & allRowsHeight. Maybe to viewportHeight & contentHeight? Likewise for columns / width.

This makes much more sense, renamed these.

rtrg · 2019-12-17T11:24:07Z

I also noticed that the Reorderable Columns example seems broken on this branch and doesn't show the unfrozen columns. Could you take a look and see what the issue might be? Thanks.

Thanks for pointing out, missed passing scrollbarYWidth prop to row. This is fixed now.

wcjordan

This looks good from my end. @pradeepnschrodinger & @mycrobe do you have any concerns?

mycrobe · 2020-01-07T14:34:00Z

no concerns from me

wcjordan · 2020-01-15T16:00:04Z

@pradeepnschrodinger do you have any concerns here? If not can we go ahead and merge?

Side note, does it make sense to start an 1.1 branch at target this for that so master can handle any v1.0 bug fixes? Or start a 1.0 branch for bug fixes?

pradeepnschrodinger · 2020-01-17T00:34:17Z

@pradeepnschrodinger do you have any concerns here? If not can we go ahead and merge?

This looks very good. I think I'll take a deeper look by end of this week.
Sorry for the big delays.

Side note, does it make sense to start an 1.1 branch at target this for that so master can handle any v1.0 bug fixes? Or start a 1.0 branch for bug fixes?

Yea, let's do a feature branch for custom scrollbars.
Currently master doesn't seem to have a lot of bugs, so let's keep it that way until we have a stable support for custom scrollbars (maybe with some docs and examples to finish off before merging).

This way we will also avoid the case where users complain about any API changes because they had already started using custom scrollbars in an early state through master.

pradeepnschrodinger

Just a few comments.

pradeepnschrodinger · 2020-01-20T10:17:41Z

src/plugins/ScrollContainer.js

+    };
+
+    _onVerticalScroll = (/*number*/ scrollPos) => {
+        if (this.state.scrollToY) {


Will this work if user scrolls to value 0 ?
I think it's better to check for undefined instead (if the check is required at all).

(similar for _onHorizontalScroll)

will take care of this

Actually we are checking this.state.scrollToY and not scrollPos, this should be fine but still took care of comparing with undefined

pradeepnschrodinger · 2020-01-20T10:25:04Z

src/FixedDataTable.js

@@ -613,6 +642,10 @@ class FixedDataTable extends React.Component {
    this._contentHeight = contentHeight;
  }

+  shouldComponentUpdate(nextProps) {


FixedDataTable has a few objects being passed as props. So I suggest removing the shouldComponentUpdate check here, especially since it's not required by the PR.

If we are absolutely certain that the passed in objects are immutable and that they won't change in the future, then we could have this in.

pradeepnschrodinger · 2020-01-20T10:34:38Z

src/FixedDataTableContainer.js

+          </ScrollContainer>
+      );
+    }
+    return fdt;


I'm a bit confused on how the custom scrollbars get "props" at this level.

When defaultScrollbars is on, we pass this.props to the default scrollbars by

if (this.props.defaultScrollbars) { return ( <ScrollContainer {...this.props}> {fdt} </ScrollContainer> ); }

Similarly shouldn't we somehow also pass it to the custom scrollbars? Maybe we should avoid passing of props alltogether at this level, and ensure that <FixedDataTable /> passes everything that's required through onScrollBarsUpdate ?

here we are passing props to ScrollContainer to render FDT and also pass touchScrollEnabled and isRTL prop

in case of custom scrollbar people will have access to these props (as they are the one passing them to FDT) and can pass directly to scrollbars if required.

pradeepnschrodinger · 2020-01-20T10:36:15Z

src/reducers/index.js

@@ -259,6 +259,8 @@ function setStateFromProps(state, props) {
  newState.tableSize.useMaxHeight =
    newState.tableSize.height === undefined;

+  newState.scrollbarXHeight = props.scrollbarXHeight;


could you also add these entries to the getInitialState() function in reducers/index.js?

Should I keep default values as Scrollbar.SIZE?

took care of this

pradeepnschrodinger · 2020-01-20T10:56:12Z

I'm a bit concerned on how the ScrollContainer component contains the FixedDataTable component, along with the fact that FixedDataTable has to manually update ScrollContainer as a side effect to it's own update.

Whenever user scrolls, a redux action is dispatched and the store is updated.
The updates in the view components goes as follows:
Store -> FixedDataTableContainer -> ScrollContainer -> FixedDataTable

Now this is fine, but the FixedDataTable will again update ScrollContainer's underlying state as a sideeffect through onScrollBarsUpdate. This'll cause yet another rerender:
ScrollContainer -> FixedDataTable

FixedDataTable is quite heavy and an extra render is something we should avoid. Also I'm not sure how fast it's to do these updates through componentDidUpdate.

Instead of all this, can't we ensure a one directional render flow by rendering the scrollbars within FixedDataTable itself?
FixedDataTableContainer -> FixedDataTable -> ScrollBars
FixedDataTable can simply pass all the scroll state details as props in this flow.

wcjordan · 2020-01-20T23:12:49Z

@pradeepnschrodinger if we have FDT render the scrollbars itself, will that make it difficult to compose? Different scrollbar components will likely have different APIs and interactions. If we can hoist the composition outside of FDT and require the end user to compose them, then they can handle the integration between the details FDT exposes and the APIs of the scrollbars.

I'm guessing the concerns you've raised around re-rendering are part of why we're looking to add shouldComponentUpdate. Could we take steps to address your concerns with adding shouldComponentUpdate checks?

rtrg · 2020-01-27T05:36:48Z

Regarding re-render concerns, can we extract out wrapper divfixedDataTableLayout/main (which is responsible for rendering scrollbars) out of FixedDataTable.js and then have ShouldComponentUpdate check in FixedDataTable?

With this, even if scrollbar state changes it doesn't re-render FixedDataTable.js and updates only scrollbars.

wcjordan · 2020-01-29T04:44:09Z

How about we include the shouldComponentUpdate check in FixedDataTable, but make it so it always returns true by default, but can be configured to do the comparison if the feature flag is set to use it? Would that work @pradeepnschrodinger ? It seems like it wouldn't break existing table users, but would let us move towards the more optimized approach and get testing of this new scrollbar API.

wcjordan · 2020-02-12T16:39:54Z

@rtrg @pradeepnschrodinger anything remaining to be done here before we merge?

rtrg · 2020-02-13T12:33:53Z

@rtrg @pradeepnschrodinger anything remaining to be done here before we merge?

Took care of minor comments from my side

pradeepnschrodinger

Looks good to me. Apologies for the delays and thanks for follow ups :)

pradeepnschrodinger · 2020-02-18T16:07:28Z

@wcjordan , as offlined some time ago, should we cut off master and make branch v1.1 ? We'll merge this PR into it, and continue any related work on the v1.1 branch.

wcjordan · 2020-02-18T16:21:24Z

Yep. Although now that I think about it, maybe we should cut v1.0 and have 1.1 work go to master? That would reflect the branching model we use for LiveDesign and may be less confusing.

To disable fdt default scrollbars, need to pass defaultScrollbars prop as false Can pass custom scrollbars by passing scrollbarX, scrollbarY. To update custom scrollbars state or trigger fdt scroll, pass onScrollBarsUpdate prop (function) which gets necessary arguments.

wcjordan · 2020-02-26T20:31:35Z

v1.0 branch has now been cut. Any concerns with me merging this to master?

pradeepnschrodinger · 2020-02-27T07:06:16Z

No concerns from my side, thanks.

wcjordan · 2020-03-02T16:31:38Z

Awesome. I've merged!

@rtrg any thoughts on what next steps we should look at? Should we look to add an example using native scroll?

rtrg · 2020-03-03T07:02:11Z

Awesome. I've merged!

Thanks!

@rtrg any thoughts on what next steps we should look at? Should we look to add an example using native scroll?

Yes, I will raise a PR for that soon.

wcjordan · 2020-04-14T15:13:39Z

This is now released w/ v1.1.

wcjordan · 2020-06-04T17:34:39Z

@rtrg any chance you have any documentation or an example putting this new API to use which we could add to the project?

wcjordan requested changes Dec 16, 2019

View reviewed changes

rtrg force-pushed the master branch from d23e2d8 to 9c250ce Compare December 17, 2019 11:15

rtrg force-pushed the master branch from 9c250ce to e980453 Compare December 18, 2019 07:35

wcjordan requested review from pradeepnschrodinger, mycrobe and somavara January 6, 2020 14:38

wcjordan approved these changes Jan 6, 2020

View reviewed changes

pradeepnschrodinger requested changes Jan 20, 2020

View reviewed changes

rtrg force-pushed the master branch from e980453 to 8a4c221 Compare February 13, 2020 13:30

pradeepnschrodinger approved these changes Feb 18, 2020

View reviewed changes

rtrg force-pushed the master branch from 8a4c221 to 5d69eac Compare February 26, 2020 08:43

wcjordan merged commit 534d060 into schrodinger:master Mar 2, 2020

wcjordan mentioned this pull request Mar 3, 2020

Support customizable / native scroll #518

Closed

wcjordan added this to the v1.1 milestone Mar 3, 2020

boxfoot mentioned this pull request Mar 9, 2020

Remove hasteResolver in favor of standard esm relative imports [Fix #513] #519

Merged

5 tasks

sharma-rishi mentioned this pull request May 29, 2020

Custom Resize and Reorder Plugins #542

Closed

7 tasks

pradeepnschrodinger mentioned this pull request Sep 25, 2020

Using Webkit ScrollBar with fixed-data-table-2 #558

Closed

wcjordan mentioned this pull request Jan 6, 2021

Is there any way to show scrollbar only when mouse over to table. #563

Closed

Ability to provide custom scrollbars: #507

Ability to provide custom scrollbars: #507

Conversation

rtrg commented Dec 10, 2019 • edited by sharma-rishi Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

wcjordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtrg commented Dec 17, 2019

wcjordan left a comment

Choose a reason for hiding this comment

mycrobe commented Jan 7, 2020

wcjordan commented Jan 15, 2020

pradeepnschrodinger commented Jan 17, 2020

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepnschrodinger commented Jan 20, 2020

wcjordan commented Jan 20, 2020

rtrg commented Jan 27, 2020

wcjordan commented Jan 29, 2020

wcjordan commented Feb 12, 2020

rtrg commented Feb 13, 2020

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

pradeepnschrodinger commented Feb 18, 2020

wcjordan commented Feb 18, 2020

wcjordan commented Feb 26, 2020

pradeepnschrodinger commented Feb 27, 2020

wcjordan commented Mar 2, 2020

rtrg commented Mar 3, 2020

wcjordan commented Apr 14, 2020

wcjordan commented Jun 4, 2020

rtrg commented Dec 10, 2019 •

edited by sharma-rishi

Loading