Cycle through headings on Ctrl+H #110
Conversation
✅ Deploy Preview for nifty-lalande-39c157 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
@b-kelly I have already added an e2e test for Ctrl + H - do I need to add more tests? |
There was a problem hiding this comment.
Left a few comments on the approach. I'd also like to add this functionality to markdown mode as well.
Regarding unit vs e2e tests - we'd like to prefer unit tests whenever possible, especially for functionality that doesn't need a view to run (as is the case here with the state commands). Unit tests are much faster and can be made much more complex / complete.
| @@ -96,6 +96,51 @@ function toggleBlockType( | |||
| }; | |||
| } | |||
|
|
|||
| const headingInfo = { | |||
There was a problem hiding this comment.
It looks like this causes the header cycling state to be shared across different heading nodes. Something to note, this will also cause state to be shared across editor instances, across heading nodes.
| time: 0, | ||
| }; | ||
|
|
||
| export function cycleThroughHeadings(nodeType: NodeType) { |
There was a problem hiding this comment.
nit: Add /** */ docs to all new + exported functions
|
|
||
| export function cycleThroughHeadings(nodeType: NodeType) { | ||
| return (state: EditorState, dispatch: (tr: Transaction) => void) => { | ||
| const delay = Date.now() - headingInfo.time; |
There was a problem hiding this comment.
I'd prefer to not check for delays in our underlying code. I think this is to support successive presses causing cycling, but single presses causing toggle? If that's something we'd like to pursue, let's move that logic into the keybind file instead
Closes #45, closes #105