Skip to content

fix(SmartPicker): Insert smart picker links as preview per default - #5846

Merged
mejo- merged 1 commit into
mainfrom
fix/smart_picker_preview
Jun 18, 2024
Merged

fix(SmartPicker): Insert smart picker links as preview per default#5846
mejo- merged 1 commit into
mainfrom
fix/smart_picker_preview

Conversation

@mejo-

@mejo-mejo- commented May 29, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Summary

Behaviour:

  • Inserts a link preview with smart picker from the toolbar if editor selection is empty
  • Inserts a text link with smart picker from the toolbar if editor selection is not empty
  • Always inserts a link preview with smart picker from / command as it's impossible to have ane editor selection when typing /

🖼️ Screenshots

🏡 Screencast
recording1

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests

@mejo-mejo- added this to the Nextcloud 30 milestone May 29, 2024
@mejo-
mejo- requested a review from max-nextcloudMay 29, 2024 15:12
@mejo-mejo- self-assigned this May 29, 2024
@mejo-mejo- changed the title fix(SmartPicker): Show smart picker links as preview per defaultfix(SmartPicker): Insert smart picker links as preview per defaultMay 31, 2024
juliusknorr
juliusknorr reviewed Jun 5, 2024
View reviewed changes
Comment threadsrc/components/Menu/ActionInsertLink.vue Outdated
@juliusknorr

juliusknorr commented Jun 5, 2024

Copy link
Copy Markdown
Member

Can we also do this when using the smart picker with typing / ?

@mejo-
mejo-force-pushed the fix/smart_picker_preview branch from f53e454 to f88e45eCompareJune 18, 2024 11:12
@mejo-
fix(SmartPicker): Show smart picker links as preview per default
bc5f65c
Fixes: #5838
Signed-off-by: Jonas <jonas@freesources.org>
@mejo-
mejo-force-pushed the fix/smart_picker_preview branch from f88e45e to bc5f65cCompareJune 18, 2024 13:46
@mejo-mejo- added 3. to review enhancement New feature or request and removed 2. developing labels Jun 18, 2024
@mejo-

mejo- commented Jun 18, 2024

Copy link
Copy Markdown
MemberAuthor

Can we also do this when using the smart picker with typing / ?

Yep, implemented now.

@mejo-
mejo- marked this pull request as ready for review June 18, 2024 13:54
@mejo-
mejo- requested review from enjeck and juliusknorrJune 18, 2024 14:03
juliusknorr
juliusknorr approved these changes Jun 18, 2024
View reviewed changes

@juliusknorrjuliusknorr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very nice, just two questions, not blocking

Comment threadcypress/e2e/nodes/Preview.spec.js
Comment threadsrc/nodes/Preview.js
attrs: { href: link, title: 'preview' },
content: [{
type: 'text',
marks: [{

@juliusknorrjuliusknorrJun 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems a bit unexpected that the preview node still has an additional mark, can you tell why?

@max-nextcloudmax-nextcloudJun 18, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it matches the data structure created otherwise. We basically change the wrapping block node between paragraph and preview when switching between the two:

The setPreview function above converts the surrounding paragraph of a link into a preview node:
grafik

If you look at unsetPreview above it also only unwraps the link inside the preview node:
grafik

max-nextcloud
max-nextcloud approved these changes Jun 18, 2024
View reviewed changes

@max-nextcloudmax-nextcloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great!

@mejo-

mejo- commented Jun 18, 2024

Copy link
Copy Markdown
MemberAuthor

/backport to stable29

@mejo-
mejo- merged commit 00cc04e into mainJun 18, 2024
@mejo-
mejo- deleted the fix/smart_picker_preview branch June 18, 2024 16:15
@backportbotbackportbotBot mentioned this pull request Jun 18, 2024
Merged
@nextcloudnextcloud deleted a comment from backportbotBotJun 18, 2024
mejo- added a commit that referenced this pull request Jun 19, 2024
@mejo-
fix(LinkPicker): Fix inserting markdown (e.g. from text_templates app)
8c23b1a
Follow-up for #5846
Signed-off-by: Jonas <jonas@freesources.org>
@mejo-mejo- mentioned this pull request Jun 19, 2024
Merged
2 tasks
backportbotBot pushed a commit that referenced this pull request Jun 24, 2024
@mejo-@backportbot
fix(LinkPicker): Fix inserting markdown (e.g. from text_templates app)
db9a879
Follow-up for #5846
Signed-off-by: Jonas <jonas@freesources.org>
mejo- added a commit that referenced this pull request Jun 26, 2024
@mejo-
fix(LinkPicker): Fix inserting markdown (e.g. from text_templates app)
5f11075
Follow-up for #5846
Signed-off-by: Jonas <jonas@freesources.org>
@blizzzblizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorrjuliusknorrjuliusknorr approved these changes
@max-nextcloudmax-nextcloudmax-nextcloud approved these changes
@enjeckenjeckAwaiting requested review from enjeck

Assignees

@mejo-mejo-

Labels

3. to reviewenhancementNew feature or request

Projects

📝 Productivity team
Archived in project

Milestone

Nextcloud 30

Development

Successfully merging this pull request may close these issues.

Display links inserted from smart picker as preview per default

3 participants

@mejo-@juliusknorr@max-nextcloud