Improve keyboard navigation in filtering #2964

sstoyanovIG · 2018-11-07T15:54:05Z

bkulov · 2018-11-08T09:01:38Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-cell.component.ts

        };
    }

    /**
     * Focus a chip depending on the current visible template.
     */
-    public focusChip() {
+    public focusChip(fucusFirst?: boolean) {


typo - focus instead of fucus

bkulov · 2018-11-08T09:07:09Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-cell.component.ts

+                    this.moreIcon.nativeElement.focus();
+                }
+            } else if (this.filteringService.focusNext) {
+                if (this.isMoreIconHidden() === false && this.chipsArea.chipsList.length === 0) {


Use !this.isMoreIconHidden() instead of this.isMoreIconHidden() === false.

bkulov · 2018-11-08T09:20:31Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-cell.component.ts

        };
    }

    /**
     * Focus a chip depending on the current visible template.
     */
-    public focusChip() {
+    public focusChip(fucusFirst?: boolean) {


This method has very high cyclomatic complexity (12). The good practice is to keep cyclomatic complexity below 6-7. Think of refactoring (extract code to other methods, combine if/else clauses, etc.) the method to avoid so many if/else expressions.
https://scottlilly.com/cyclomatic-complexity-in-visual-studio/

bkulov · 2018-11-08T09:45:24Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-cell.component.ts

        };
    }

    /**
     * Focus a chip depending on the current visible template.
     */
-    public focusChip() {
+    public focusChip(fucusFirst?: boolean) {


This method has very high cyclomatic complexity (12). Consider refactoring this method by extracting code to other methods, joining if/else clauses, etc.

bkulov · 2018-11-08T09:47:48Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-cell.component.ts

        };
    }

    /**
     * Focus a chip depending on the current visible template.
     */
-    public focusChip() {
+    public focusChip(fucusFirst?: boolean) {


It's better to provide a default value for the optional parameters. This makes the code using this parameter simpler and avoids misunderstandings.
fucusFirst?: boolean => fucusFirst: boolean = false.

bkulov · 2018-11-08T11:52:17Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-row.component.ts

@@ -338,6 +342,9 @@ export class IgxGridFilteringRowComponent implements AfterViewInit, OnDestroy {
            });
        }

+        this.filteringService.grid.filterCellList.find(cell => cell.column === this.filteringService.filteredColumn).updateFilterCellArea();


This has to be done in the filtering service. It's not appropriate one component to directly manipulate another.
In the filtering service there is method updateFilteringCell which does the same (have to be made public). You could add another to focus a chip.

bkulov · 2018-11-08T11:56:36Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering.service.ts

                    });
                }
+                if (this.columnToFocus) {
+                    this.grid.filterCellList.find(cell => cell.column === this.columnToFocus).focusChip();


Use the newly created method from my previous comment.

bkulov · 2018-11-08T12:03:34Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering.service.ts


    public gridId: string;
    public isFilterRowVisible = false;
    public filteredColumn = null;
    public selectedExpression: IFilteringExpression = null;
-    public columnToChipToFocus = new Map<string, boolean>();
+    public columnToFocus = null;
+    public focusNext = false;


Perhaps more meaningful name would be something like shouldFocusNext or shouldFocusRight.

bkulov · 2018-11-08T12:06:55Z

projects/igniteui-angular/src/lib/grids/grid-navigation.service.ts

+                    this.grid.rowList.first.cells.first._clearCellSelection();
+                    const visColLength = this.grid.visibleColumns.length;
+                    if (this.grid.headerContainer.getItemCountInView() === visColLength) {
+                        this.grid.filterCellList.last.focusChip();


This should be done through the new method in the filtering service.

bkulov · 2018-11-08T12:13:14Z

projects/igniteui-angular/src/lib/grids/filtering/grid-filtering-cell.component.ts

+
+    private ScrollToChip(columnIndex: number, scrollRight: boolean) {
+        this.filteringService.grid.nativeElement.focus({preventScroll: true});
+        this.filteringService.columnToFocus = this.filteringService.grid.visibleColumns[columnIndex];


Setting columnToFocus and focusNext happens only in filtering cell component and in navigation service. And in the both places these two fields are set together and are never read. So in order to make the code a little bit easier to use and error prone we could create a method in the filtering service to set those two fields. They are only used in the service so make them private.

focusNext is read in the filtering cell when we decide in which direction we should move the focus.

npaunov · 2018-11-08T14:11:26Z

Several scenarios:

Pinned Columns, Go to Column Moving Demo. Click Pin ContactName. Click on the first cell in the grid.

Click shift and tab. The focus does not move on the last chip.
Click on the last chip, hold shift and tab. The focus does not go to the first chip.

Grouped Columns. Group a column, click on the first cell of the grid. Click shift + tab. The focus does not move to the last chip.

* feat(grid, chip): add display density DI token to igxGrid and igxChip * Resolving confilcts related to the new structure(grids ->grid) * Fixing styling issue * Adding event arguments for DenistyChanged * Renaming IDisplayDenisty to IDisplayDenistyOptions and setting IDensityChangedEventArgs type to be DisplayDenisty * Trigger onDensityChanged only for components that do not have displayDenisty explicitly set when the global denisty is changed * Adding displayDenisty DI in tree grid constructor * Group by chips are now affected from the global display denisty option * fix(igxGrid): dirty check only checks if value exists, #2940 * fix(igx-grid): Open date-picker only if condition is not unary, #2937 * Summaries display denisty classes are now correctly applied * Removing dsiplayDenisty property since it is not needed any more * fix(igx-grid): Clear filter when value is cleared from input, #2945 * test(treeGrid): Update treeGrid Keyboard navigation tests #2953 * Improve keyboard navigation in filtering (#2964) * fix(igx-grid): Improve keyboard navigation in filtering, #2951 * fix(igx-grid): Get scroll from first row, when there is one, #2951 * chore(igx-grid): Refactor code, #2951 * chore(igx-grid): Add cases for groupbyrow and pinned columns, #2951 * chore(igx-grid): Fix lint errors, #2951 * feat(igx-grid): Close filter row on Esc, #2979 * test(igx-grid): Add test, #2979 * chore(*): Merging master into 6.2.x * Sorting strategy merge 6.2.x (#2998) * feat(grid sorting): Merging the new sorting strategy into 6.2.x #2734 * chore(*): Adding missing imports to grid-selection tests * chore(*): More test import fixes * chore(*): Applying the groupby changes with the new sorting strategy * chore(*): Resolving the column moving test issue * chore(*): This should be it with the test fixes * chore(*): Removing the fdescribe

* feat(grid, chip): add display density DI token to igxGrid and igxChip * Resolving confilcts related to the new structure(grids ->grid) * Fixing styling issue * Adding event arguments for DenistyChanged * Renaming IDisplayDenisty to IDisplayDenistyOptions and setting IDensityChangedEventArgs type to be DisplayDenisty * Trigger onDensityChanged only for components that do not have displayDenisty explicitly set when the global denisty is changed * Adding displayDenisty DI in tree grid constructor * Group by chips are now affected from the global display denisty option * fix(igxGrid): dirty check only checks if value exists, #2940 * fix(igx-grid): Open date-picker only if condition is not unary, #2937 * Summaries display denisty classes are now correctly applied * Removing dsiplayDenisty property since it is not needed any more * fix(igx-grid): Clear filter when value is cleared from input, #2945 * test(treeGrid): Update treeGrid Keyboard navigation tests #2953 * Improve keyboard navigation in filtering (#2964) * fix(igx-grid): Improve keyboard navigation in filtering, #2951 * fix(igx-grid): Get scroll from first row, when there is one, #2951 * chore(igx-grid): Refactor code, #2951 * chore(igx-grid): Add cases for groupbyrow and pinned columns, #2951 * chore(igx-grid): Fix lint errors, #2951 * feat(igx-grid): Close filter row on Esc, #2979 * test(igx-grid): Add test, #2979 * chore(*): Merging master into 6.2.x * Sorting strategy merge 6.2.x (#2998) * feat(grid sorting): Merging the new sorting strategy into 6.2.x #2734 * chore(*): Adding missing imports to grid-selection tests * chore(*): More test import fixes * chore(*): Applying the groupby changes with the new sorting strategy * chore(*): Resolving the column moving test issue * chore(*): This should be it with the test fixes * chore(*): Removing the fdescribe * Add igxDropDown maxHeight input (#3005) * feat(igxDropDown): introduce itemsMaxHeight property #3001 * test(igxDropDown): test for max height #3001 * docs(igxDropDown): changelog update #3001 * test(igxCombo): adjust tests according to new dropdown structure #3001 * test(igxDropDown): add more tests #3001 * fix(grid): MultiColumn headers should not be grouped #2944 (#2970) * chore(*): Fixing the demos * feat(themes): add theme schemas (#3006) * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): create schemas for more comopnents * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): create schemas for more comopnents * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): create schemas for more comopnents * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): add schemas for the remaining component themes * refactor(themes): change presets to schemas * fix(themes): value resolver always returns string * refactor(themes): remove all $default-theme references * docs(themes): add docs for newly introduced functions * refactor(demos): update demos theme * build(themes): fix css theme not building * refactor(themes): optimize palette generation * refactor(combo) - navigation (#2999) * refactor(combo): refactor combo navigation, WIP * refactor(combo): refactor combo navigation * refactor(combo): add comments, trim, remove console.log * refactor(combo): add comments, trim, remove console.log * chore(combo): adjust failing tests * refactor(igxCombo): remove focusItem, use super.navigateItem instead * refactor(igxCombo): add navigate virtual * refactor(igxCombo): add focus add button + findFocusableIndex methods * refactor(igxCombo): adjust incorrect `navigateItem` calls * refactor(igxCombo): first header is not loaded and focus search - scroll, #2999 * The grid should update groupsRecords before raising the onGroupingDone event - 6.2.x, #2967 (#3009) * fix(groupBy): update the state before raising onGroupingDone event #2967 * fix(groupBy): calling detectChanges instead of ngDoCheck #2967

* feat(grid, chip): add display density DI token to igxGrid and igxChip * Resolving confilcts related to the new structure(grids ->grid) * Fixing styling issue * Adding event arguments for DenistyChanged * Renaming IDisplayDenisty to IDisplayDenistyOptions and setting IDensityChangedEventArgs type to be DisplayDenisty * Trigger onDensityChanged only for components that do not have displayDenisty explicitly set when the global denisty is changed * Adding displayDenisty DI in tree grid constructor * Group by chips are now affected from the global display denisty option * fix(igxGrid): dirty check only checks if value exists, #2940 * fix(igx-grid): Open date-picker only if condition is not unary, #2937 * Summaries display denisty classes are now correctly applied * Removing dsiplayDenisty property since it is not needed any more * fix(igx-grid): Clear filter when value is cleared from input, #2945 * test(treeGrid): Update treeGrid Keyboard navigation tests #2953 * Improve keyboard navigation in filtering (#2964) * fix(igx-grid): Improve keyboard navigation in filtering, #2951 * fix(igx-grid): Get scroll from first row, when there is one, #2951 * chore(igx-grid): Refactor code, #2951 * chore(igx-grid): Add cases for groupbyrow and pinned columns, #2951 * chore(igx-grid): Fix lint errors, #2951 * fix(grid): update method _updateVScrollOffset to calculate top #2957 * fix(grid): update lint #2957 * feat(igx-grid): Close filter row on Esc, #2979 * fix(grid): Recalculate start index on _applyChanges #2957 * fix(grid): Update lint #2957 * test(igx-grid): Add test, #2979 * fix(IgxTreeGrid): refocus row when expand/collapse tree row #2935 * fix(IgxGrid): shound not toggle expand on arrowLeft when row is expanded #2950 * chore(*): Merging master into 6.2.x * Sorting strategy merge 6.2.x (#2998) * feat(grid sorting): Merging the new sorting strategy into 6.2.x #2734 * chore(*): Adding missing imports to grid-selection tests * chore(*): More test import fixes * chore(*): Applying the groupby changes with the new sorting strategy * chore(*): Resolving the column moving test issue * chore(*): This should be it with the test fixes * chore(*): Removing the fdescribe * Add igxDropDown maxHeight input (#3005) * feat(igxDropDown): introduce itemsMaxHeight property #3001 * test(igxDropDown): test for max height #3001 * docs(igxDropDown): changelog update #3001 * test(igxCombo): adjust tests according to new dropdown structure #3001 * test(igxDropDown): add more tests #3001 * fix(grid): MultiColumn headers should not be grouped #2944 (#2970) * chore(*): Fixing the demos * feat(themes): add theme schemas (#3006) * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): create schemas for more comopnents * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): create schemas for more comopnents * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): create schemas for more comopnents * refactor(themes): introduce presets as basis for component themes * refactor(themes): create schemas for more comopnents * refactor(themes): add schemas for the remaining component themes * refactor(themes): change presets to schemas * fix(themes): value resolver always returns string * refactor(themes): remove all $default-theme references * docs(themes): add docs for newly introduced functions * refactor(demos): update demos theme * build(themes): fix css theme not building * refactor(themes): optimize palette generation * refactor(combo) - navigation (#2999) * refactor(combo): refactor combo navigation, WIP * refactor(combo): refactor combo navigation * refactor(combo): add comments, trim, remove console.log * refactor(combo): add comments, trim, remove console.log * chore(combo): adjust failing tests * refactor(igxCombo): remove focusItem, use super.navigateItem instead * refactor(igxCombo): add navigate virtual * refactor(igxCombo): add focus add button + findFocusableIndex methods * refactor(igxCombo): adjust incorrect `navigateItem` calls * refactor(igxCombo): first header is not loaded and focus search - scroll, #2999 * The grid should update groupsRecords before raising the onGroupingDone event - 6.2.x, #2967 (#3009) * fix(groupBy): update the state before raising onGroupingDone event #2967 * fix(groupBy): calling detectChanges instead of ngDoCheck #2967 * fix(grid): #2507 prevent scrolling on arrow keys (#2989) * fix(grid): #2507 prevent scrolling on arrow keys * fix(grid): #2507 prevent scrolling for default columns only * Feat - Combo - Add DisplayDensity Input to Combo and Drop Down (#3007) * feat(igxCombo): add display density to combo component, refactor template,#2410 * feat(igxCombo): add hidden to binding, add display: block, fix tests, #2410 * feat(igxCombo): move display styling in styles, remove hostbinding, #2410 * chore(*): Updating the PR template (#3036) * chore(*): Updating the PR template * chore(*): Addressing Stamen's comment * fix(grid): removing checks which prevent scrolling #2971 (#3022)

fix(igx-grid): Improve keyboard navigation in filtering, #2951

bd73494

sstoyanovIG added ❌ status: awaiting-test PRs awaiting manual verification grid: general keyboard-navigation version: 6.2.x filtering labels Nov 7, 2018

sstoyanovIG assigned npaunov Nov 7, 2018

sstoyanovIG requested a review from bkulov November 7, 2018 15:54

fix(igx-grid): Get scroll from first row, when there is one, #2951

35ad755

npaunov added 💥 status: in-test PRs currently being tested ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Nov 8, 2018

bkulov suggested changes Nov 8, 2018

View reviewed changes

chore(igx-grid): Refactor code, #2951

938d241

npaunov added 🛠️ status: in-development Issues and PRs with active development on them and removed ✅ status: verified Applies to PRs that have passed manual verification labels Nov 8, 2018

bkulov previously approved these changes Nov 8, 2018

View reviewed changes

npaunov added 💥 status: in-test PRs currently being tested and removed 🛠️ status: in-development Issues and PRs with active development on them labels Nov 8, 2018

Merge branch '6.2.x' into sstoyanov/bug-fix-#2951

acb1c82

chore(igx-grid): Add cases for groupbyrow and pinned columns, #2951

00cc67a

sstoyanovIG dismissed bkulov’s stale review via 00cc67a November 8, 2018 15:37

npaunov added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Nov 8, 2018

bkulov previously approved these changes Nov 9, 2018

View reviewed changes

bkulov and others added 2 commits November 9, 2018 10:09

Merge branch '6.2.x' into sstoyanov/bug-fix-#2951

3d10f2e

chore(igx-grid): Fix lint errors, #2951

947daa4

sstoyanovIG dismissed bkulov’s stale review via 947daa4 November 9, 2018 08:32

bkulov approved these changes Nov 9, 2018

View reviewed changes

kdinev merged commit ab3b06e into 6.2.x Nov 9, 2018

kdinev deleted the sstoyanov/bug-fix-#2951 branch November 9, 2018 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve keyboard navigation in filtering #2964

Improve keyboard navigation in filtering #2964

sstoyanovIG commented Nov 7, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

bkulov Nov 8, 2018

sstoyanovIG Nov 8, 2018

npaunov commented Nov 8, 2018

Improve keyboard navigation in filtering #2964

Improve keyboard navigation in filtering #2964

Conversation

sstoyanovIG commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npaunov commented Nov 8, 2018