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

t/ckeditor5/1514: Created a guide with solutions for common conversion problems reported by the users #1677

Merged
merged 25 commits into from
Sep 10, 2019

Conversation

oleq
Copy link
Member

@oleq oleq commented Feb 13, 2019

Suggested merge commit message (convention)

Docs: Created a guide with solutions for common conversion problems reported by the users. Closes ckeditor/ckeditor5#1514.


Additional information

Requires ckeditor/ckeditor5#1520 to get the link UI working in the examples upon clicking a link.

Some comments

  • The number of examples is limited. OTOH they are very time–consuming to prepare.
  • The guide does not cover lists and tables. A complex conversion is used there I didn't have time to research.
  • I feel that even with an introduction, this guide is very hard to read and understand. It asks for a proper "comprehensive conversion guide" but it is a huuuuuuge topic.
    • If we write a comprehensive guide in the future, we should move the intro part from this guide there and merge it.

@oleq oleq requested a review from Reinmar February 13, 2019 14:31
@coveralls
Copy link

coveralls commented Feb 13, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling ebae49d on t/ckeditor5/1514 into 62fe3e1 on master.

@Reinmar
Copy link
Member

Reinmar commented Feb 13, 2019

More ideas – there are some really cool examples in https://github.com/ckeditor/ckeditor5-core/blob/poc/catch-all-converters/tests/manual/converters.js and https://github.com/ckeditor/ckeditor5-core/blob/poc/catch-all-converters/tests/manual/converters.html. We need to add them to the list :)

Also... we need tests for the converters in these samples :((( But I have an idea how to do that ;>

@jodator
Copy link
Contributor

jodator commented Feb 14, 2019

I'll add those to the PR.

@oleq
Copy link
Member Author

oleq commented Feb 14, 2019

The same thing as d370243 may be necessary for other snippets too. They work without it but it could be an accident that during writer.wrap() the result is the old element that had the custom prop already defined.

See ckeditor/ckeditor5#1532.

}
} )
.then( editor => {
window.editor = editor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we copy-paste a lot :)

Shouldn't we namespace those somehow? Right now the last snippet executed on the page "wins".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Probably window.editors = { ... }.

@jodator
Copy link
Contributor

jodator commented Feb 18, 2019

@Reinmar & @oleq: I've forgot to push the second example with fonts. I think that we can have that one only from the two that were created as they seems too similar to me. But I'm happy to add the section with converting font-names also if you see it helpful.

@Reinmar
Copy link
Member

Reinmar commented Jun 28, 2019

One more case to cover: divs with classes:

  editor.model.schema.register( 'div', {
      allowWhere: '$block',
      allowContentOf: '$block', // May also be `$root` if it's supposed to behave as a wrapper.
      allowAttributes: 'class'
  } );

  // Model-to-view convert for the div element (attrbiutes are converted separately).
  editor.conversion.elementToElement( {
      model: 'div',
      view: 'div'
  } );

  editor.conversion.attributeToAttribute( {
      model: { name: 'div', key: 'class' },
      view: 'class'
  } );

@Reinmar
Copy link
Member

Reinmar commented Jun 28, 2019

Also, we actually have to review many of those examples now because of link decorators. I wouldn't necessary remove them, but explain that this can also be achieved with that feature.

@jodator
Copy link
Contributor

jodator commented Jul 17, 2019

I've added:

  • small fixes to the docs (mainly written by me)
  • references to the link decorators

Examples cross-linked to the link decorators:

  • Adding a CSS class to inline elements
  • Adding an HTML attribute to certain inline elements
  • Adding a CSS class to certain inline elements
  • Loading content with a custom attribute

The code works AFAICS and the examples are still relevant but some of them are already implemented as link decoreators so we might think in the future to rewrite them to similar cases but withouth using < href="...">.

Still the whole guide is just needed to be published IMO :)

@Reinmar
Copy link
Member

Reinmar commented Jul 17, 2019

Great job, @jodator! Thanks!

I guess we still have some things that we could add to this guide. We've been doing some PoC's in ckeditor5-core, e.g. Could you also open a followup ticket with links to those and some other ideas that we posted here or that come to your head? You've been implementing many strange cases for our customers, so there must be something :D

@jodator
Copy link
Contributor

jodator commented Jul 18, 2019

@Reinmar most of the ideas from this PR and a ticket are implemented in the guide. Anyway, the follow up is here: https://github.com/ckeditor/ckeditor5-engine/issues/1761 as I couldn't find more ideas.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

All the imports (Font, Plugin, Code) need to be moved to build-extending-content-source.js and exposed there.

@Reinmar Reinmar self-assigned this Jul 23, 2019
@Reinmar
Copy link
Member

Reinmar commented Sep 9, 2019

6th attempt at reviewing this :D

@Reinmar
Copy link
Member

Reinmar commented Sep 10, 2019

OK, I admit I didn't check the code samples themselves but rather focused on gluing together some intro and making this guide easier to follow by splitting it. Let's just polish it with time.

Anyway, thanks for all the content guys. Lots of valuable information will finally be available to the people.

@Reinmar Reinmar merged commit 17c495f into master Sep 10, 2019
@Reinmar Reinmar deleted the t/ckeditor5/1514 branch September 10, 2019 10:07
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.

[Docs] Write a guide solving common conversion problems reported by the users
4 participants