ConvertKit/slate-plugins

lists: Add query for being inside list

Opened this issue · 8 comments

This is useful for e.g. button bars
Not sure what to call it, isList, hasList, inList?

	queries: {
		isList: editor => !!getListItem(editor, editor.value.startBlock)
	}

getListItem will then need a guard for undefined block

Or maybe one for UL and one for OL

I like inList. Maybe an argument for the type since the block type could be overridden? It could default to either.

editor.inList({ type: "unordered-list" })
editor.inList({ type: "ordered-list" })
editor.inList() // either

well - should the type be the one that is passed as the block type, or should it be the key of the block type? In my case, that would be 'UL' vs 'unordered_list'

Also, do we anticipate any other arguments ever? If not, maybe passing just the type as a string suffices; later on the argument type could still be detected if objects are needed.

It would be whatever you passed in as the block type, so 'UL' in your case.

Good question on other arguments, I'm not sure. Currently, wrapList currently takes an object with the type key. I'm not sure whether or not consistency with that is applicable. Personally, I like type: "unordered-list" in wrapList, but I could go either way with inList.

Hmm. I took another look at the plugin and I have a couple of remarks:

  • There are two semi-hardcoded list types, ordered and unordered, but there is no reason for that. How about accepting an array of block types that encode lists, defaulting to types: ['ordered-list', 'unordered-list'] (['UL', 'OL'] in my case). That way, we're not constrained by the arbitrary HTML limits (e.g. a sorted list type that orders the list by some property of the children).
  • The list item type could then default to item: 'list-item' ('LI' for me)
  • Why is there a list_item_child as well as a list_item? Is it to enforce the schema? In that case, it's like an internal node, which shouldn't be exposed to the user? But is it really necessary to enforce that schema?
  • Since commands like wrapList etc take only the type, it would be more efficient to just accept the string type. I keep thinking of the poor VM that needs to create an object and GC it again just for that one string :)

There are two semi-hardcoded list types, ordered and unordered, but there is no reason for that. How about accepting an array of block types that encode lists, defaulting to types: ['ordered-list', 'unordered-list'] (['UL', 'OL'] in my case). That way, we're not constrained by the arbitrary HTML limits (e.g. a sorted list type that orders the list by some property of the children).

I wonder if this could be accomplished by using the plugin more than once. In that case, the plugin would only have to handle one type. I chose to hard code it for now to keep it simpler, but it doesn't have to stay that way.

Why is there a list_item_child as well as a list_item? Is it to enforce the schema? In that case, it's like an internal node, which shouldn't be exposed to the user? But is it really necessary to enforce that schema?

It made it easier to make work out of the box. A list-item can have text and/or another list as children. But Slate can't put a text node and a block node next to each other, so we need a block to hold the text if another list is in there. It's exposed to the user because you could just have that be a paragraph, or whatever else you want.

Since commands like wrapList etc take only the type, it would be more efficient to just accept the string type. I keep thinking of the poor VM that needs to create an object and GC it again just for that one string :)

I'm not too worried about the efficiency here. If there was a benchmark of a real world use case where it made a difference, then maybe we could change it for that reason. I'm open to changing it for other reasons, but probably further down the line.

I wonder if this could be accomplished by using the plugin more than once. In that case, the plugin would only have to handle one type. I chose to hard code it for now to keep it simpler, but it doesn't have to stay that way.

Hmm, but then there will be two inList queries, and the plugin won't know to keep the list when switching from one to another.

But Slate can't put a text node and a block node next to each other

Does Slate require each node to be a real DOM node? Maybe list_item could be just the block type but it just renders its children unchanged?

For the wrapList, I prefer reading wrapList('UL') versus wrapList({type: 'UL'}). Personal preference, I know, but it would be nice to be able to do that.

Good point about multiple queries. I personally don't have a use case for more than two list types right now, so I don't plan on adding it, but would be open to a PR for it if someone needs it.

Does Slate require each node to be a real DOM node?

I thought so, but I'm not sure. The slate attributes have to get put into the DOM somehow.

Regarding the wrapList command, the thought was that I'd add support for passing other attributes, like wrapList({ type: "UL", data: {}}). I don't think I'd be opposed to supporting both though as that mimics what slate does: https://docs.slatejs.org/slate-core/commands#wrapblock

It may be easier to discuss some of these things in separate issues.