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

Introduced todo lists. #133

Merged
merged 44 commits into from
Aug 13, 2019
Merged

Introduced todo lists. #133

merged 44 commits into from
Aug 13, 2019

Conversation

oskarwrobel
Copy link
Contributor

@oskarwrobel oskarwrobel commented Aug 8, 2019

Suggested merge commit message (convention)

Feature: Introduced todo lists. Closes ckeditor/ckeditor5#1434.


TODO before merge:

  • improve v -> m conversion for data pipeline
  • use command for toggling check state after clicking on checkbox
  • add and improve API docs
  • increase CC to 100% https://github.com/ckeditor/ckeditor5-list/issues/126
  • do not use Renderer for getting changed checkmark elements
  • improve manual test

@coveralls
Copy link

coveralls commented Aug 8, 2019

Coverage Status

Coverage remained the same at ?% when pulling 7020e67 on t/1434 into a792961 on master.

@Mgsy Mgsy self-requested a review August 9, 2019 10:56
src/listediting.js Outdated Show resolved Hide resolved
src/todolistcheckcommand.js Outdated Show resolved Hide resolved
src/todolistconverters.js Outdated Show resolved Hide resolved
src/todolistconverters.js Outdated Show resolved Hide resolved
@pjasiun
Copy link

pjasiun commented Aug 9, 2019

  1. Select: list + todo list + list
  2. press ctrl + space

Result:

position.js:362 Uncaught TypeError: Cannot read property 'is' of undefined
    at Function._createAfter (position.js:362)
    at DowncastWriter.createPositionAfter (downcastwriter.js:956)
    at DowncastDispatcher.<anonymous> (todolistconverters.js:186)
    at DowncastDispatcher.fire (emittermixin.js:207)
    at DowncastDispatcher._testAndFire (downcastdispatcher.js:466)
    at DowncastDispatcher.convertAttribute (downcastdispatcher.js:240)
    at DowncastDispatcher.convertChanges (downcastdispatcher.js:141)
    at editingcontroller.js:90
    at View.change (view.js:467)
    at Document.EditingController.listenTo.priority (editingcontroller.js:89)

bug

@oskarwrobel
Copy link
Contributor Author

oskarwrobel commented Aug 11, 2019

  1. Select: list + todo list + list
  2. press ctrl + space

Result:

position.js:362 Uncaught TypeError: Cannot read property 'is' of undefined
    at Function._createAfter (position.js:362)
    at DowncastWriter.createPositionAfter (downcastwriter.js:956)
    at DowncastDispatcher.<anonymous> (todolistconverters.js:186)
    at DowncastDispatcher.fire (emittermixin.js:207)
    at DowncastDispatcher._testAndFire (downcastdispatcher.js:466)
    at DowncastDispatcher.convertAttribute (downcastdispatcher.js:240)
    at DowncastDispatcher.convertChanges (downcastdispatcher.js:141)
    at editingcontroller.js:90
    at View.change (view.js:467)
    at Document.EditingController.listenTo.priority (editingcontroller.js:89)

bug

That's strange. Unit tests cover this case with no exception. No, it's not :)

Fixed.

@oskarwrobel
Copy link
Contributor Author

You can't select two items in a table cell.

bug_cke5

Edit: Actually, it's a different case. The selection collapses after reaching a checkbox. On longer words it works fine.

bug_cke5

Fixed.

@oskarwrobel
Copy link
Contributor Author

Steps to reproduce

  1. Create two todo list items.
  2. Check the second one.
  3. Select both items.
  4. Convert it to an ordered list.

Current result

The editor crashes.

Fixed.

src/todolistediting.js Outdated Show resolved Hide resolved
src/todolistediting.js Outdated Show resolved Hide resolved
@oskarwrobel oskarwrobel marked this pull request as ready for review August 13, 2019 09:22
Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

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

It looks good to me 👍

@Mgsy
Copy link
Member

Mgsy commented Aug 13, 2019

During testing a collaboration I've noticed that shortcut for checking list on OS X triggers changing the language. We should consider if Ctrl + Space is a good idea. cc @pjasiun @oskarwrobel

Edit: It turned out that I had enabled some system shortcuts which aren't enabled by default. After talking with @pjasiun, we decided to leave it as it is.

tests/manual/todo-list.md Outdated Show resolved Hide resolved
@pjasiun pjasiun merged commit 6b12658 into master Aug 13, 2019
@pjasiun pjasiun deleted the t/1434 branch August 13, 2019 13:51
pjasiun pushed a commit that referenced this pull request Aug 13, 2019
Feature: Introduced to-do lists. Closes ckeditor/ckeditor5#1434.

writer.setAttribute( 'listType', 'todo', modelItem );

if ( data.viewItem.getAttribute( 'checked' ) == 'checked' ) {

Choose a reason for hiding this comment

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

This fails when upcasting checkboxes with binary attributes input type="checkbox" checked which are parsed as checked = "". So this would need to check against an empty string as well.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't data.viewItem.checked be enough?

Copy link

Choose a reason for hiding this comment

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

It will be solved in the follow-up ticket https://github.com/ckeditor/ckeditor5-list/issues/138.

Choose a reason for hiding this comment

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

@pomek Yes that should work

Great thanks! Also, awesome work on the tasklist 🍻

@kidrahahjo
Copy link

@oskarwrobel Hey, I have a question regarding creating an interactive checklist for a GitHub's pull request template. Do you have any idea how this can be done?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants