Cell Cut Copy Paste Highlight for Iframe2#755
Cell Cut Copy Paste Highlight for Iframe2#755djbarnwal wants to merge 1 commit intoiodide-project:iframe-2from
Conversation
bcolloran
left a comment
bcolloran
left a comment
There was a problem hiding this comment.
very nice PR @djbarnwal! it works wonderfully well, and will be a big usability improvement. i have a handful of requests, but nothing too big -- in a couple places that i didn't understand, i just might need an explanation rather than an actual change.
in addition to the comments inline in the code:
- 'esc' should remove highlighting
- make sure that all comments about files on the editor or eval pane side are mirrored to the other side
| if (!this.props.selected) { | ||
| this.props.selectCell(this.props.cellId, scrollToCell) | ||
| handleCellClick = (event) => { | ||
| if (!event.target.classList.contains('CodeMirror-line')) { |
There was a problem hiding this comment.
is this conditional meant to prevent clicks in the text editor from enabling highlighting? if so, it's not really working for me (clicks in the text editor are still enabling the highlighting). if that is the intent, we could just limit highlight clicks to the cell header (since that would be pretty much the only click area left). You could create a new handler handleCellHeader and attach it to the cell-header div.
But i'm not 100% sure what your objective was here, so that may not work depending on what you were trying to accomplish
| lineNumbers: true, | ||
| keyMap: 'sublime', | ||
| comment: codeMirrorMode === 'javascript', | ||
| readOnly: getReadOnly(), |
There was a problem hiding this comment.
any reason you define this function then call it immediately? would it work just set the value of readOnly directly, or am i missing something?
|
|
||
| case 'UPDATE_CELL_PROPERTIES': | ||
| case 'UPDATE_CELL_PROPERTIES': { | ||
| const approvedMultipleChanges = new Set(['skipInRunAll']) |
There was a problem hiding this comment.
the const approvedMultipleChanges can go at the top of the file (it's kind of a global) and it should have a brief comment describing what it is. also i think it can probably just be a plain list rather than a set.
|
|
||
| case 'UNHIGHLIGHT_CELLS': { | ||
| const cells = state.cells.slice() | ||
| cells.forEach((c) => { c.highlighted = false }) // eslint-disable-line |
There was a problem hiding this comment.
if you're disabling eslint, specify the rule being disabled
There was a problem hiding this comment.
in this case, you can avoid disabling the eslint and skip the cells.slice above using a map (which accomplishes the same copying as the slice). something along the lines of:
const cells = state.cells.map((c) =>Object.assign({}, c, { highlighted: false }))
| const high = Math.max(index1, index2) | ||
| cells.forEach((c, index) => { | ||
| if (low <= index && index <= high) { | ||
| c.highlighted = true // eslint-disable-line |
| if (low <= index && index <= high) { | ||
| c.highlighted = true // eslint-disable-line | ||
| } else { | ||
| c.highlighted = false // eslint-disable-line |
| cutCells: { | ||
| type: 'array', | ||
| items: cellSchema, | ||
| }, |
There was a problem hiding this comment.
is it necessary to have both copiedCells and cutCells (rather than, for example, just a single cellClipboard)? it may well be necessary, and perhaps i havenot understood the code deeply enough to see why both are needed.
|
@djbarnwal can you push your local branch of this again? |
2 participants
Ports features of PR #629 into Iframe 2
Implements cut, copy, paste, highlight, unhighlight, actions using multiple-highlight (for ex. skip in Run all at once for multiple cells)