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

WIP: Add functionality to insert new block after the last block #85

Merged
merged 10 commits into from
Jan 4, 2024

Conversation

flolbr
Copy link
Contributor

@flolbr flolbr commented Dec 27, 2023

This adds a shortcut (Alt+Enter) to create a new block at the end of the buffer

@heyman
Copy link
Owner

heyman commented Dec 27, 2023

Thanks!

There are now two PRs for creating new blocks at different locations in the buffer (this one and #34). I think it would be a good idea to have the following key bindings:

C-Enter: New block after current
Alt-Enter: New block before current

And adding Shift to those adds the block to either the beginning or the end of the whole buffer instead:

C-Shift-Enter: New block at the end of buffer
Alt-Shift-Enter: New block at the beginning of buffer

Currently, C-Shift-Enter is the keyboard shortcut for splitting the current block at the cursor position, so that would have to be rebound to C-Alt-Enter.


Would you be interested in creating the other key bindings as well (possibly merging #34 into this PR)?

Also, it should be fairly simple to create tests for this. Look at the existing tests for reference.

@flolbr
Copy link
Contributor Author

flolbr commented Dec 27, 2023

I could absolutely merge both into this one, and try to write the tests as well.

The only thing I'm questioning, is whether the "simpler" shortcuts should be used to create at the beginning/end of the buffer, and the +Shift ones for before/after block. If feel that, for at least my use-case, they are more often used.

I'll prepare everything else while waiting for your opinion.

@flolbr flolbr changed the title Add functionality to insert new block after the last block WIP: Add functionality to insert new block after the last block Dec 27, 2023
@heyman
Copy link
Owner

heyman commented Dec 27, 2023

Cool!

I'm mainly using the one that creates a new block after the current one, so I'd like to keep the non-shift keybindings for those. Once proper keybinding support is implemented, users can change that themselves.

@heyman
Copy link
Owner

heyman commented Dec 27, 2023

I added info to the Readme on how to run the tests: https://github.com/heyman/heynote?tab=readme-ov-file#run-tests

I found the test UI pretty nice because you'll get to see the actual Heynote UI that your're testing. However, I did find that there were some cases where the tests passed in the UI but not in the CLI runner (missing await IIRC), so make sure to run the tests in the CLI as well once they pass.

@flolbr flolbr marked this pull request as draft December 28, 2023 15:33
@flolbr flolbr marked this pull request as ready for review December 28, 2023 20:00
Copy link

@fabjan fabjan left a comment

Choose a reason for hiding this comment

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

Nice, and with tests to boot!

Since my PR is getting absorbed into this one I'll butt in with a short review even if nobody asked:

  • The README is missing two of the new commands
  • The README also has the wrong binding for "Split the current block at cursor position"
  • initial-content.ts has the same two issues as the README has
  • There are two console.log calls left in, maybe from debugging
  • There is TODO left undone, about moving the cursor

Do with this info what you want, it's your PR!

src/editor/block/commands.js Outdated Show resolved Hide resolved
tests/block-creation.spec.js Outdated Show resolved Hide resolved
tests/block-creation.spec.js Outdated Show resolved Hide resolved
@gerroon
Copy link

gerroon commented Jan 1, 2024

Does anyone know when this will be in?

@heyman
Copy link
Owner

heyman commented Jan 1, 2024

Nice work @flolbr and @fabjan! I've fixed the visual bug, and updated the help text and Readme.

This should be good to merge now, yes?

@flolbr
Copy link
Contributor Author

flolbr commented Jan 1, 2024

Not yet @heyman, I've made other changes and added more edge-case tests.
Thank you for fixing the visual bug. Have you checked if it also fixes this ?

@flolbr flolbr force-pushed the add-block-end branch 2 times, most recently from 905888a to bf1ce2f Compare January 1, 2024 18:26
@heyman
Copy link
Owner

heyman commented Jan 1, 2024

Have you checked if it also fixes this ?

I've now pushed another fix for that.

flolbr and others added 5 commits January 1, 2024 22:18
- Update key bindings in `initial-content.ts` to include `Alt + Enter` for adding a new block after the last block.
- Implement `getLastNoteBlock` function in `block.js` to retrieve the last block in the note.
- Add `addNewBlockAfterLast` command in `commands.js` to handle the insertion of a new block after the last one.
- Integrate `addNewBlockAfterLast` command into the keymap in `keymap.js`.
…. Also, tests.

- Added `getFirstNoteBlock` in `block.js` for accessing the first text block.
- Implemented new functions in `commands.js` like `addNewBlockBeforeCurrent` and `addNewBlockBeforeFirst`.
- Updated `keymap.js` with new key bindings to facilitate block creation.
- Introduced `block-creation.spec.js` for testing the new block manipulation features.
…e buffer, when the previous first block's delimiter is long (e.g. Markdown)
- Add a documentation generator
- Add an option to force the initial content to be erased with an env variable
@flolbr
Copy link
Contributor Author

flolbr commented Jan 1, 2024

I think that's the one @heyman.

The main change of the last commits is writing a generator for the keybind helper, which gets fed to the initial content and the README. I guess it could also be used for a future docs tab in the settings.

@fabjan
Copy link

fabjan commented Jan 1, 2024

It all seems to be working great now as I test your branch with npm run dev 👍

The playwright tests however are not green when I run them on my Mac, because page.isMac is undefined som the Meta key is never used for the shortcuts. Looks like isMac is added by the HeynotePage helper class from testutils. If I wrap page from the test code in that the isMac check works and the tests are green for me:

    // create a new block
    let modKey = new HeynotePage(page).isMac ? "Meta" : "Control";
    await page.locator("body").press(key.replace("Mod", modKey));

@fabjan
Copy link

fabjan commented Jan 1, 2024

@heyman regarding the shortcuts, opt+shift is the default modifiers used by the Amethyst window manager. I ran into a collision already with opt+shift+F for format, in that case I just disabled the corresponding shortcut in Amethyst because I didn't use it. opt+shift+return however is the shortcut for making the focused window the main window in the layout, which I do all the time.

I don't know how common the window manager is and maybe you prefer to deal with issues from users later, should they arise, instad of trying to find a perfect shortcut. Anything will collide with something I guess.

@flolbr
Copy link
Contributor Author

flolbr commented Jan 1, 2024

It all seems to be working great now as I test your branch with npm run dev 👍

The playwright tests however are not green when I run them on my Mac, because page.isMac is undefined som the Meta key is never used for the shortcuts. Looks like isMac is added by the HeynotePage helper class from testutils. If I wrap page from the test code in that the isMac check works and the tests are green for me:

    // create a new block
    let modKey = new HeynotePage(page).isMac ? "Meta" : "Control";
    await page.locator("body").press(key.replace("Mod", modKey));

Try to apply this patch and let me know if it works
isMac.patch

@fabjan
Copy link

fabjan commented Jan 2, 2024

Try to apply this patch and let me know if it works

Oh, that also works: the failing tests go green as the mod key gets set correctly 👍

It seems like at least "create block before first Markdown block" is flaky though, I saw it fail once but unfortunately didn't grab a screenshot. I'm wondering if the shared top level heynotePage can cause flakiness: https://playwright.dev/docs/browser-contexts and https://playwright.dev/docs/test-retries#reuse-single-page-between-tests but I cannot provoke the fail after many many retries now, so it may not be an issue.

@heyman
Copy link
Owner

heyman commented Jan 4, 2024

I don't know how common the window manager is and maybe you prefer to deal with issues from users later, should they arise, instad of trying to find a perfect shortcut. Anything will collide with something I guess.

Yeah, I think the solution to this is to implement proper support for customizing key-bindings.

@heyman
Copy link
Owner

heyman commented Jan 4, 2024

Is this ready to merge now @flolbr ?

@flolbr
Copy link
Contributor Author

flolbr commented Jan 4, 2024

LGTM

@heyman heyman merged commit d0d8f87 into heyman:main Jan 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants