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] Show focus cell appropriately when enableFocus becomes true #1447

Merged
merged 7 commits into from
Aug 30, 2017

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Aug 15, 2017

Fixes #1443

Checklist

  • Include tests

Changes proposed in this pull request:

  • Fixed Changing Table enableFocus prop to true now shows a focused cell right away.

Reviewers should focus on:

  • All the tests look at the component state, rather than what is rendered. Seemed easier to check the exact coordinates of the focused cell that way.

Screenshot

2017-08-14 17 13 15

@cmslewis cmslewis added this to the 1.26.0 milestone Aug 15, 2017
@blueprint-bot
Copy link

Show focus cell appropriately when enableFocus becomes true

Preview: documentation | table
Coverage: core | datetime

@cmslewis cmslewis removed this from the 1.26.0 milestone Aug 15, 2017
} else {
// focus the top-left cell of the table
newFocusedCellCoordinates = { col: 0, row: 0, focusSelectionIndex: 0 };
}
Copy link
Contributor

@giladgray giladgray Aug 15, 2017

Choose a reason for hiding this comment

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

might be nice to pull this out to getInitialFocusCell helper that simply returns in each if instead of assigning to a let variable?

then it's just const newFocusedCellCoordinates = this.getInitialFocusCell(args);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like it's time to introduce focusUtils.ts (per https://github.com/palantir/blueprint/pull/1496/files#r135396325). 😎

@blueprint-bot
Copy link

Oops, delete .only

Preview: documentation | table
Coverage: core | datetime

@@ -0,0 +1,3 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

needs file header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@blueprint-bot
Copy link

Add missing file header

Preview: documentation | table
Coverage: core | datetime

@cmslewis cmslewis dismissed giladgray’s stale review August 30, 2017 16:20

Addressed missing file header

@cmslewis cmslewis merged commit 537acca into master Aug 30, 2017
@cmslewis cmslewis deleted the cl/enable-focus-cell branch August 30, 2017 16:20
@cmslewis cmslewis mentioned this pull request Aug 30, 2017
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.

4 participants