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

fix(src): handle new form of list item as paragraph - I192 #194

Merged
merged 14 commits into from
Nov 18, 2019

Conversation

irmerk
Copy link
Member

@irmerk irmerk commented Nov 15, 2019

Issue #192

Adjust the handling of lists with the new paragraph support in markdown-transform v0.8

Changes

  • Keep focus when clicking a block in the toolbar
  • Handle block types as constants
  • Handle breakout of empty list item
  • Modularize and refactor code
    • Block action methods
    • onClickBlock handler
    • Handle lists onEnter in the plugin

Flags

  • Work still needed for this PR
    • Persisting issue around block_quote to list_item, requires further work
    • Formatting Toolbar buttons don't yet recognize the selection being in a list
  • Future Issues
    • Work around SHIFT + ENTER handling a linebreak / new paragraph in a list_item is needed

Related Issues

  • N/A

@irmerk irmerk added Type: Bug 🐛 Something isn't working Difficulty: Medium Type: Enhancement ✨ Improvement to process or efficiency labels Nov 15, 2019
@irmerk irmerk self-assigned this Nov 15, 2019
@irmerk irmerk added this to In Progress PR in Cicero UI v1.0 via automation Nov 15, 2019
@irmerk
Copy link
Member Author

irmerk commented Nov 15, 2019

Netlify deployment

event.preventDefault();
editor.splitBlock().unwrapBlock(CONST.LIST_ITEM).wrapBlock(CONST.LIST_ITEM);
});
return true;
Copy link
Member

Choose a reason for hiding this comment

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

What is the effect of this? Shouldn't it return next() instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think next() is resulting in there being a paragraph in the list_item and a new paragraph below, as if a sibling of the list_item.

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 confused about this as well. What should be the type returned by onEnter ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unsure if it should be returning a type. @dselman you may have some insight?

Copy link
Member

Choose a reason for hiding this comment

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

What is .unwrapBlock(CONST.LIST_ITEM).wrapBlock(CONST.LIST_ITEM) doing on line 37? Why would we unwrap a list item just to wrap it again? moved comment to correct line

Signed-off-by: irmerk <jolenelanglinais@gmail.com>
… - I192

Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

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

Some questions.

@@ -89,8 +89,6 @@ const schema = {
nodes: [
{
match: [
{ object: 'text' },
{ type: 'link' },
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this include lists as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so, but I think that should be left for another issue when we add support for nested lists.

src/plugins/list.js Outdated Show resolved Hide resolved
event.preventDefault();
editor.splitBlock().unwrapBlock(CONST.LIST_ITEM).wrapBlock(CONST.LIST_ITEM);
});
return true;
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 confused about this as well. What should be the type returned by onEnter ?

Signed-off-by: irmerk <jolenelanglinais@gmail.com>
Signed-off-by: irmerk <jolenelanglinais@gmail.com>
if (isSelectionInput(value, CONST.LIST_ITEM) && startBlock.text.length !== 0) {
editor.withoutNormalizing(() => {
event.preventDefault();
editor.splitBlock().unwrapBlock(CONST.LIST_ITEM).wrapBlock(CONST.LIST_ITEM);
Copy link
Member

Choose a reason for hiding this comment

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

What is .unwrapBlock(CONST.LIST_ITEM).wrapBlock(CONST.LIST_ITEM) doing on line 37? Why would we unwrap a list item just to wrap it again?

Copy link
Member Author

@irmerk irmerk Nov 18, 2019

Choose a reason for hiding this comment

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

I made this diagram that hopefully helps explain the process. Essentially when splitBlock is called, there are two child paragraph nodes under a list_item. So now the list_item is acting as if it itself an ul_list. When unwrapBlock is called, it moves this new node from the split up one level, but now it is not under a list_item. So wrapping back puts it under a list_item which stems from the ol_list

Image from iOS (1)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explaining this. Makes sense now!

@irmerk irmerk merged commit f946b38 into accordproject:master Nov 18, 2019
Cicero UI v1.0 automation moved this from In Progress PR to Finished PR Nov 18, 2019
@irmerk irmerk deleted the irmerk/i192/persist-lists branch November 18, 2019 20:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Medium Type: Bug 🐛 Something isn't working Type: Enhancement ✨ Improvement to process or efficiency
Projects
No open projects
Cicero UI v1.0
  
Finished PR
Development

Successfully merging this pull request may close these issues.

None yet

4 participants