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

Add options to hide MultiGrid top-right and bottom-left grid scrollbars but still make the scroll events work on those grids #1040

Merged
merged 10 commits into from
May 15, 2018

Conversation

RaviDasari
Copy link
Contributor

@RaviDasari RaviDasari commented Mar 7, 2018

The main issue here is that we want scroll events to work on top-right and bottom-left grids but dont want those scrollbars. I fixed this issue with simple css and an extra container. This stackoverflow is the inspiration for my solution. here is how it looks - observe that in the below demo, I could use mouse scroll to scroll through the MultiGrid though the top-right and bottom-left grids doesn't show any scrollbars.

image
I tested this in latest chrome, firefox, edge and IE11 and it works. in short, I am simply hiding the scrollbar by adding an extra container. So I am sure this works in all browsers.

@RaviDasari RaviDasari changed the title Hide multigrid scrollbars Add options to hide MultiGrid top-right and bottom-left grid scrollbars Mar 7, 2018
@RaviDasari
Copy link
Contributor Author

I am running tests and will add tests soon. @bvaughn what do you think of this fix?

...this._topRightGridStyle,
height,
width,
overflowX: 'hidden',
Copy link
Contributor

@wuweiweiwu wuweiweiwu Mar 12, 2018

Choose a reason for hiding this comment

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

I don't think you need to wrap it in another div

If you need to. you can just modify this._topRightGridStyle

this._topRightGridStyle = {
   ...this._topRightGridStyle,
   height,
   width, 
   overflowX: 'hidden'
}

Unless Im misunderstanding something :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We need this extra wrapper so we can hide the scrollbar with a hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wuweiweiwu , are you waiting for some change here?

@RaviDasari RaviDasari changed the title Add options to hide MultiGrid top-right and bottom-left grid scrollbars Add options to hide MultiGrid top-right and bottom-left grid scrollbars but still make the scroll events work on those grids Mar 12, 2018
const additionalColumnCount = showHorizontalScrollbar ? 1 : 0,
height = this._getTopGridHeight(props),
width = this._getRightGridWidth(props),
scrollbarSize = this.state.showHorizontalScrollbar ? this.state.scrollbarSize : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: showHorizontalScrollbar is already deconstructed above. scrollbarSize can also be put there.

@@ -47,6 +49,8 @@ export default class MultiGrid extends React.PureComponent {
styleBottomRightGrid: {},
styleTopLeftGrid: {},
styleTopRightGrid: {},
hideTopRightGridScrollbar: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think there is a need to specify defaultProps for these. Since it would be undefined if not passed in. Which is a falsey value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added because there was an existing pattern for enableFixedColumnScroll and enableFixedRowScroll. So followed it.

@mattpage
Copy link

What's the status on this PR?

@RaviDasari
Copy link
Contributor Author

Was busy with work, will update the PR soon.

@RaviDasari
Copy link
Contributor Author

@wuweiweiwu , I think I addressed all the comments and added tests.

Also added docs.

@DGulshan
Copy link

Any updates on this PR?

@wuweiweiwu
Copy link
Contributor

@RaviDasari @DGulshan looks good to me. I want to wait for #1053 to get in first

@wuweiweiwu
Copy link
Contributor

I'll to make sure that the test pass with the new changes in Grid. but it looks good so far!

Copy link
Contributor

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

can you also make sure yarn lint passes?

const additionalRowCount = showVerticalScrollbar ? 1 : 0,
height = this._getBottomGridHeight(props),
width = this._getLeftGridWidth(props),
scrollbarSize = this.state.showVerticalScrollbar ? this.state.scrollbarSize : 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

scrollbarSize is now in state.instanceProps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think state structure change in Grid but not in MutliGrid so it is working fine.
Also, I ran yarn lint and it seem to be all good too.

So we are good here?

@RaviDasari
Copy link
Contributor Author

RaviDasari commented May 14, 2018

@wuweiweiwu looks like my code had couple of lint errors, fixed them along with few other issues I found in other parts of the code. Though it is irrelevant as part of this PR but I hope it is fine.

Update: all tests are green.

@wuweiweiwu
Copy link
Contributor

Looks good! tests pass. I think this can land in the 9.19.0 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants