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

rename overscan props for Gird with deprecation #229

Merged
merged 2 commits into from
May 27, 2019

Conversation

nihgwu
Copy link
Contributor

@nihgwu nihgwu commented Apr 30, 2019

close #226

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This looks solid. One small request to split up the deprecation message if you're willing.

);
expect(console.warn).toHaveBeenCalledTimes(1);
expect(console.warn).toHaveBeenLastCalledWith(
'The overscanCount prop has been deprecated. ' +
'Please use the overscanColumnsCount and overscanRowsCount props instead.'
'The overscanCount, overscanColumnsCount and overscanRowsCount props have been deprecated. ' +
Copy link
Owner

Choose a reason for hiding this comment

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

I think this message may be a bit confusing for people who were unaware of the old overscanCount prop. Seems like it would be better to split these into two warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will do

@nihgwu
Copy link
Contributor Author

nihgwu commented May 1, 2019

Hey @bvaughn , I'd like to ask for your help for an off-topic problem, Autodesk/react-base-table#22, do you have any idea to avoid a second rendering to solve the problem, that is if I know it's a pure client rendering I could just use the real scrollbar size in construction, or I have to update it in cDM

I also found that in react-virtualized https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.js#L610 this statement is wrong, and it won't work actually, as in client hydration the component's constructor render will be called again and the scrollbar size is measured there, so this block won't be hit at all

I noticed this problem after I migrated to react-window, it wasn't a problem as I always do a second rendering because I relied on onScrollbarPresenceChange from Grid before, but now I calculate it by myself for migrating to react-window

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Thanks!

Please use the <code>overscanRowCount</code> property instead.
</p>
),
name: 'overscanRowsCount',
Copy link
Owner

Choose a reason for hiding this comment

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

I think you forgot to rename overscanRowsCount -> overscanRowCount for the pre-existing one. 😁 I'll do it!

@bvaughn bvaughn merged commit 5d7fb35 into bvaughn:master May 27, 2019
@bvaughn
Copy link
Owner

bvaughn commented May 28, 2019

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

Successfully merging this pull request may close these issues.

overscanRowsCount vs overscanRowCount
2 participants