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

Extend Grid API to allow custom keyboard navigation #4646

Merged
merged 45 commits into from
May 10, 2019

Conversation

ddincheva
Copy link
Contributor

@ddincheva ddincheva commented Apr 24, 2019

Closes #4054

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code
  • This PR includes API docs for newly added methods/properties
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes
  • This PR includes behavioral changes and the feature specification has been updated with them

@zdrawku
Copy link
Contributor

zdrawku commented May 8, 2019

After initial testing, everything looks good. I have only two comments below:

  • update the Changelog, version 7.2.8 and add a bullet for igxGrid Keyboard custom navigation and below list the changes
  • rename perFormAction private method to navigate or something more relevant to the internal navigation with row and col index

@ddincheva ddincheva requested a review from rkaraivanov May 8, 2019 12:17
@ddincheva ddincheva added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels May 8, 2019
@nrobakova nrobakova added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels May 8, 2019
CHANGELOG.md Outdated
- `onFocusChange` event is deprecated.
- `onGridKeydown` is exposed. The event will emit
`IGridKeydownEventArgs { targetType: GridKeydownTargetType; target: Object; event: Event; cancel: boolean; }`
- `navigateTo(rowIndex: number, visibleColumnIndex: number, callback({targetType, target: Object }))` - this method allows you to navigate to a randon position in the grid;
Copy link
Contributor

@zdrawku zdrawku May 8, 2019

Choose a reason for hiding this comment

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

Suggested change
- `navigateTo(rowIndex: number, visibleColumnIndex: number, callback({targetType, target: Object }))` - this method allows you to navigate to a randon position in the grid;
- `navigateTo(rowIndex: number, visibleColumnIndex: number, callback({targetType, target: Object }))` - this method allows you to navigate to a position in the grid based on provided `rowindex` and `visibleColumnIndex`;

Copy link
Contributor

@zdrawku zdrawku left a comment

Choose a reason for hiding this comment

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

Update navigteTo description

@nrobakova nrobakova added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels May 10, 2019
@zdrawku zdrawku merged commit 07ac332 into 7.2.x May 10, 2019
@zdrawku zdrawku deleted the ddincheva/customKBNav branch May 10, 2019 12:48
return nextRow ? this.verticalScrollContainer.igxForOf.indexOf(nextRow) : currentRowIndex;
}

private isValidPosition(rowIndex, colIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't have return type

}
}

public getPreviousCell(currRowIndex: number, curVisibleColIndex: number, callback: (IgxColumnComponent) => boolean = null) {
Copy link
Contributor

@mpavlinov mpavlinov May 10, 2019

Choose a reason for hiding this comment

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

Public API. Doesn't have description and code snippet.
Also there is no return type. You should introduce a new interface.

}
}

public getNextCell(currRowIndex: number, curVisibleColIndex: number, callback: (IgxColumnComponent) => boolean = null) {
Copy link
Contributor

@mpavlinov mpavlinov May 10, 2019

Choose a reason for hiding this comment

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

Public API. Doesn't have description and code snippet.
Also there is no return type. You should introduce a new interface.

@@ -4884,6 +4905,136 @@ export abstract class IgxGridBaseComponent extends DisplayDensityBase implements
directive.scrollTo(goal);
}

public navigateTo(rowIndex: number, visibleColIndex = -1, cb: Function = null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Public API. Doesn't have description and code snippet.

return {rowIndex: currRowIndex, visibleColumnIndex: nextCellIndex};
} else {
if (colIndexes.length === 0 || this.getNextDataRowIndex(currRowIndex) === currRowIndex) {
return {rowIndex: this.getNextDataRowIndex(currRowIndex), visibleColumnIndex: curVisibleColIndex};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please introduce a new type/interface. We don't want anonymous objects. See "Anonymous objects is a no-no."

return {rowIndex: currRowIndex, visibleColumnIndex: prevCellIndex};
} else {
if (colIndexes.length === 0 || this.getPrevDataRowIndex(currRowIndex) === currRowIndex) {
return {rowIndex: this.getPrevDataRowIndex(currRowIndex), visibleColumnIndex: curVisibleColIndex};
Copy link
Contributor

Choose a reason for hiding this comment

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

Please introduce a new type/interface.

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