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

overscanRowsCount vs overscanRowCount #226

Closed
nihgwu opened this issue Apr 29, 2019 · 5 comments · Fixed by #229
Closed

overscanRowsCount vs overscanRowCount #226

nihgwu opened this issue Apr 29, 2019 · 5 comments · Fixed by #229
Labels
💬 question Further information is requested

Comments

@nihgwu
Copy link
Contributor

nihgwu commented Apr 29, 2019

it's overscanRowCount in react-virtualized, but now it's overscanRowsCount, while we have rowCount, I'm not a native speaker so I'm just wondering which is proper? Right now I have to map overscanRowCount to overscanRowsCount https://github.com/Autodesk/react-base-table/pull/19/files#diff-4177f614bd864f6344923aed07c2edeaR91

@bvaughn
Copy link
Owner

bvaughn commented Apr 29, 2019

I don't know if either one is more or less proper. Sorry! 😄

@bvaughn bvaughn closed this as completed Apr 29, 2019
@bvaughn bvaughn added the 💬 question Further information is requested label Apr 29, 2019
@nihgwu
Copy link
Contributor Author

nihgwu commented Apr 29, 2019

So I'm wondering if we could make the naming consisitency? like overscanRowCount

@bvaughn
Copy link
Owner

bvaughn commented Apr 29, 2019

No. react-virtualized is not a library I support anymore, so I don't plan on releasing an update to it.

And I don't think it's worth making a breaking change to react-window to better mirror react-virtualized API. That isn't really a goal this library has.

@nihgwu
Copy link
Contributor Author

nihgwu commented Apr 29, 2019

I understand you concern, I think we could deprecate overscanRowsCount in flavor of overscanRowCount, just like tagName to elementType, I'm not saying to mirror the RV's API, but the prop's name itself is confusing with rowCount

@bvaughn
Copy link
Owner

bvaughn commented Apr 29, 2019

Ah. I don't really care about mirroring RV's API naming conventions at this point, but overscanRowsCount vs rowCount is a stronger argument.

I would be willing to accept a PR that deprecates the name with a warning, similar to

if (innerTagName != null || outerTagName != null) {
if (devWarningsTagName && !devWarningsTagName.has(instance)) {
devWarningsTagName.add(instance);
console.warn(
'The innerTagName and outerTagName props have been deprecated. ' +
'Please use the innerElementType and outerElementType props instead.'
);
}
}

Interested in contributing? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants