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

[Table] Rename the onFocus prop #1562

Closed
cmslewis opened this issue Sep 13, 2017 · 6 comments
Closed

[Table] Rename the onFocus prop #1562

cmslewis opened this issue Sep 13, 2017 · 6 comments

Comments

@cmslewis
Copy link
Contributor

  • Package version(s): v1.27.0

Table's onFocus callback conflicts with DOMAttributes<T>'s onFocus callback:

  • The former is invoked when the focused cell moves.
  • The latter is invoked when a DOM element receives focus and becomes the document.activeElement.

We should probably rename Table's callback to something else to clarify the difference. Ideas:

  • onCellFocus
  • onFocusedCellChange
@cmslewis
Copy link
Contributor Author

// FYI @giladgray

@llorca
Copy link
Contributor

llorca commented Sep 13, 2017

onCellFocus SGTM

@giladgray
Copy link
Contributor

invoked when the focused cell moves

this sounds like a Change event, not a Focus event, so onFocusedCellChange is more accurate.

@adidahiya
Copy link
Contributor

this sounds like a Change event, not a Focus event, so onFocusedCellChange is more accurate.

with that logic you could argue that focus on regular DOM elements also changes the document.activeElement and reach a conclusion that it should be called onFocusChange instead of onFocus...

so I'm with @llorca here, onCellFocus SGTM

@cmslewis
Copy link
Contributor Author

Yeah, the focused cell doesn't receive literal focus, but it does get a blue box around it and serve as the point of "focus" from which tons of other behaviors emanate in the table.

Another consideration: if we went with onCellFocus, would literally focusing in an EditableCell lead to excessive conceptual/naming confusion? That's the one major case where something could be properly "focus'd" within the table.

I could go either way.

@adidahiya
Copy link
Contributor

fixed by #1890

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

No branches or pull requests

5 participants