Fix focusing of cells when navigating cells using up/down arrow #9885
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## master #9885 +/- ##
=======================================
Coverage 61.22% 61.22%
=======================================
Files 563 563
Lines 30058 30058
Branches 4545 4545
=======================================
Hits 18403 18403
Misses 10626 10626
Partials 1029 1029
Continue to review full report at Codecov.
|
| const addIndex = prevState.cellVMs.findIndex(c => c.cell.id === arg.payload.cellId); | ||
|
|
||
| const anotherCellWasFocusedAndSelected = typeof prevState.focusedCellId === 'string' && prevState.focusedCellId === prevState.selectedCellId; | ||
| const shouldSetFocusToCell = typeof shouldFocusCell === 'boolean' ? shouldFocusCell : anotherCellWasFocusedAndSelected; |
There was a problem hiding this comment.
Hold on a second, don't we already have a focus cell action? This looks like what got us into trouble before, by having functions perform their own version of an action instead of using the base action.
There was a problem hiding this comment.
To explain better, previously for defocus we had a defocus action where "stuff" had to happen. But some other actions were defocusing cells without using that action so we were not performing the correct defocusing work. This looks similar.
There was a problem hiding this comment.
With the new changes in the ds/custom_editor branch, we no longer need a defocus concept.
Basically setting focus or selecting a cell, automatically unselects and unfocuses the previously selected/focused cells.
There was a problem hiding this comment.
This is an existing action for selectCell, that also sets focus to the cell if required.
I don't think we have a focus action.
There was a problem hiding this comment.
I take that back, we do have a focus action
Looks like it needs to be cleaned up.
Today the select action also sets focus!
If you're ok with it, i'd rather fix that as a seprate PR, I can do that in the ds/custom_editor branch.
And I agree, it needs to be fixed up, to avoid this confusion (basically fix the separation of concerns... focus will deal with focus, and select with deal with select)
There was a problem hiding this comment.
A new PR is fine. Just want to make sure that we don't get into trouble like before.
IanMatthewHuff
left a comment
There was a problem hiding this comment.
One thing to discuss quick, see comment.
For #9884
Can't seem to add a functional test for monaco focus.