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

enh(ui): smart picker button at the start of line #6855

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented Jan 15, 2025

Fixes #6818.

  • Show a + button that opens the smart picker.
  • Show the button when the cursor is at the end of the line.
  • Do not show the button if the smart picker is already open.
  • Remove the placeholder text.
  • Tab navigation to the smart picker works.

🎥 Screen recording

Bildschirmaufzeichnung.vom.2025-01-15.22-52-09.mp4

TODO

  • check if showing the button while in the middle of the line and navigating to the end is an option.
  • tab navigation only works if there are no other buttons in the text (from table, preview, link paragraph)
  • add tests

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 33 lines in your changes missing coverage. Please review.

Project coverage is 46.77%. Comparing base (e22c08f) to head (cffe09b).

Files with missing lines Patch % Lines
src/plugins/currentLineMenu.js 91.50% 13 Missing ⚠️
src/plugins/previewOptions.js 64.51% 11 Missing ⚠️
src/plugins/LinkBubblePluginView.js 0.00% 6 Missing ⚠️
src/components/Editor/PreviewOptions.vue 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6855      +/-   ##
==========================================
+ Coverage   46.55%   46.77%   +0.22%     
==========================================
  Files         748      735      -13     
  Lines       34318    34457     +139     
  Branches     1242     1254      +12     
==========================================
+ Hits        15976    16118     +142     
- Misses      17720    17732      +12     
+ Partials      622      607      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@max-nextcloud max-nextcloud force-pushed the enh/smartpicker-button branch from 95a2190 to cbdb933 Compare January 15, 2025 11:31
@max-nextcloud
Copy link
Collaborator Author

I'm pondering still showing the button if one is in the middle of the line and moving the cursor to the end of the line if the button is clicked / activated. I imagine it would be nice to say change the current line into a heading with tab navigation from any point in the line.

* Show a `+` button that opens the smart picker.
* Show the button when the cursor is at the end of the line.
* Do not show the button if the smart picker is already open.
* Remove the placeholder text.
* Tab navigation to the smart picker works.

Signed-off-by: Max <[email protected]>
@max-nextcloud max-nextcloud force-pushed the enh/smartpicker-button branch from cbdb933 to 35296f7 Compare January 15, 2025 11:38
@max-nextcloud max-nextcloud changed the title enh(ui): smart picker button at the end of line enh(ui): smart picker button at the start of line Jan 15, 2025
@max-nextcloud
Copy link
Collaborator Author

I updated the screen recording to reflect the latest changes.

@juliusknorr juliusknorr added enhancement New feature or request 3. to review labels Jan 16, 2025
@juliusknorr
Copy link
Member

Pushed a small commit to use NcButton, add an aria label to prevent console warnings and fix the jest tests.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Very nice, gave it some testing and the code looks good 👍

@marcoambrosini Can you also check?

I'd also be fine with merging and doing follow ups on polishing

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I think that as soon as there's something in the current line, the plus button should either:

  • disappear (for now)
  • Open the smart picker in a new line, right after the current one

@max-nextcloud
Copy link
Collaborator Author

Open the smart picker in a new line, right after the current one

Sounds good. That would imply that if i select say "Heading" it would not turn the current line into a heading but rather start a heading below. Makes sense to me.

@@ -131,7 +131,7 @@ function currentParagraphDecorations(doc, currentParagraph, editor) {
*/
function decorationForCurrentParagraph(currentParagraph, editor) {
return Decoration.widget(
currentParagraph.pos + 1,
currentParagraph?.pos + 1,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will still cause an exception if currentParagraph is undefined, right? I'd expect undefined + 1 to raise a type error. I'll make sure this never gets called without a paragraph instead.

Also improve some type annotations.

Signed-off-by: Max <[email protected]>
@max-nextcloud max-nextcloud force-pushed the enh/smartpicker-button branch from 9f01835 to 641dda4 Compare January 16, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make smart picker button more prominent
3 participants