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

PR: Acknowledge inline widgets #70

Merged
merged 17 commits into from
Feb 18, 2019
Merged

PR: Acknowledge inline widgets #70

merged 17 commits into from
Feb 18, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jan 31, 2019

Suggested merge commit message (convention)

Other: Introduce inline widget sample. Closes ckeditor/ckeditor5#1096.


Additional information

ps.: WIP as other PRs requires tests

@jodator
Copy link
Contributor Author

jodator commented Feb 5, 2019

@Reinmar Finally done - all issues should be resolved. The go-to is a manual test with placeholder inline widget POC.

Works:

  • typing after inline widget
  • inline widget selection
  • fake selection on inline widget (selection was set in text nodes around IW in the view. I've fixed that by introducing default modelToView helper in mapper (added by editing).

The build is green on CI.

@jodator jodator requested a review from Reinmar February 5, 2019 12:50
@jodator jodator changed the title T/ckeditor5/1096 PR: Acknowledge inline widgets Feb 6, 2019
Changed inline-widget manual sample to use it.
Removed CSS-created '{}' from manual sample.
@scofalik
Copy link
Contributor

I've added mapper callback for non-empty view element -> empty model element mapping. I've modified the manual sample to use it. Also, {...} placeholder brackets are now inserted in view instead of CSS, which caused problems.


model.insertContent( placeholder );

writer.setSelection( placeholder, 'on' );
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering – shouldn't that placeholder be selected by insertContent()? If it was a block, it would be. However, if it was a block, the case is slightly different because the selection should not be set after it. So selecting it is the most reasonable solution. In case of inline objects, we can set the selection after this element (and we do that now). WDYT?

cc @scofalik @jodator

Copy link
Contributor

Choose a reason for hiding this comment

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

From UX perspective, I think that setting the selection after the inline-widget is defensible. You insert the widget and maybe want to go on with writing.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I can asnwer this myself – pasting should not select the placeholder, so should not insertContent().

@scofalik
Copy link
Contributor

BTW. I am missing one here thing, which I forgot to add. The widget has no label, resulting in no description for fake selection. I think we should encourage people to add these so it should be in a guide and I guess in the manual sample too.

@Reinmar Reinmar merged commit 38fa159 into master Feb 18, 2019
@Reinmar Reinmar deleted the t/ckeditor5/1096 branch February 18, 2019 11:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants