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

#5866: To-do lists down-casting refactored #7562

Merged
merged 13 commits into from Jul 8, 2020
Merged

#5866: To-do lists down-casting refactored #7562

merged 13 commits into from Jul 8, 2020

Conversation

niegowski
Copy link
Contributor

Suggested merge commit message (convention)

Fix (list): Editor should not crash on the enter key press inside a to-do list item containing soft-breaks. Closes #5866. Closes #6585.

Fix (list): Links inside a to-do list item should be properly converted to HTML. Closes #5779.

Feature (engine): Changed visibility scope of Mapper#findPositionIn() from private to public.


Additional information

@niegowski niegowski changed the title #5866: To-do lists downcasting refactored #5866: To-do lists down-casting refactored Jul 6, 2020
@jodator jodator self-requested a review July 7, 2020 09:33
@jodator jodator self-assigned this Jul 7, 2020
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

This works nice. I've only have two remarks about missing test for opened API method (or rename in already existing test - I didn't check that) and one minor thing in organising some callbacks.

* @param {module:engine/view/element~Element} viewParent Tree view element in which we are looking for the position.
* @param {Number} expectedOffset Expected offset.
* @returns {module:engine/view/position~Position} Found position.
*/
_findPositionIn( viewParent, expectedOffset ) {
findPositionIn( viewParent, expectedOffset ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Requires some tests when opening as a publi API.

Comment on lines 109 to 119
const direction = getLocalizedArrowKeyCodeDirection( domEventData.keyCode, editor.locale.contentLanguageDirection );

editor.keystrokes.set( localizedJumpOverCheckmarkKey, ( evt, stop ) => jumpOverCheckmarkOnSideArrowKeyPress( stop, model ) );
if ( direction != 'left' ) {
return;
}

if ( jumpOverCheckmarkOnSideArrowKeyPress( model ) ) {
domEventData.preventDefault();
domEventData.stopPropagation();
eventInfo.stop();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep that in jumpOverCheckmarkOnSideArrowKeyPress() together:

this.listenTo( editing.view.document, 'keydown', getJumpOverCheckmarkOnSideArrowKeyPress( editor ) );
// or just editor.locale.contentLanguageDirection + editor.model

so, no returning Boolean and stopping event propagation outside.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Especially since now the method returns:

  1. undefined
  2. true
  3. false

There's something strange and at least should be double checked.

@niegowski niegowski requested a review from jodator July 7, 2020 16:42
@jodator jodator merged commit 3d26015 into master Jul 8, 2020
@jodator jodator deleted the i/5866 branch July 8, 2020 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants