Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Make auto-embed insert media in-place of pasted link. #68

Merged
merged 4 commits into from
Jan 4, 2019
Merged

Make auto-embed insert media in-place of pasted link. #68

merged 4 commits into from
Jan 4, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Dec 12, 2018

Suggested merge commit message (convention)

Fix: The auto-media feature should insert media in place of pasted link. Closes ckeditor/ckeditor5#2757. Closes ckeditor/ckeditor5#2764.


Additional information

@jodator jodator requested a review from oleq December 12, 2018 12:03
@coveralls
Copy link

coveralls commented Dec 12, 2018

Pull Request Test Coverage Report for Build 289

  • 7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 280: 0.0%
Covered Lines: 302
Relevant Lines: 302

💛 - Coveralls

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

  1. Do we have a test which verifies that autoembed actually happens in place? (vide https://github.com/ckeditor/ckeditor5-media-embed/issues/49)

  2. I'm a little bit confused by the following:

    1. <paragraph>f[o]o</paragraph>
    2. autoembed
    3. Result
    <paragraph>f</paragraph>
    [<media>]
    <paragraph>o</paragraph>
    

    but

    1. <paragraph>f[o]o</paragraph>
    2. regular embed
    3. Result
    [<media>]
    <paragraph>foo</paragraph>
    

    shouldn't this be unified? Like no matter how you embed, the interaction with the existing content is the same?

@jodator
Copy link
Contributor Author

jodator commented Jan 3, 2019

shouldn't this be unified?

AFAICS this ticket is just opposed to that. It was unified but the auto-embed (the one done by pasting the URL to the content) was producing unexpected behavior. The inserting media by button is done the same way as image (the original how-to-insert-things one) and table.

@jodator
Copy link
Contributor Author

jodator commented Jan 3, 2019

@oleq to address all the issues:

  1. Yes:
    it( 'inserts media in-place', () => {

but also I've added the second one:

2c1f17f

  1. Well as I understood recent changes the media embed should work the same as other features inserting widgets like image and table so the usage with command is as it is now. On the other hand the auto media embed which replaces the pasted link with media works kinda differently as it removes link from content (it it might be placed over non-collapsed selection) with media. So the #36/#49 were reported IMO.

@oleq oleq merged commit 5f79878 into master Jan 4, 2019
@oleq oleq deleted the t/36 branch January 4, 2019 09:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants