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

lists: Add query for being inside list #25

Open
wmertens opened this issue Mar 20, 2019 · 8 comments
Open

lists: Add query for being inside list #25

wmertens opened this issue Mar 20, 2019 · 8 comments
Labels
enhancement New feature or request slate-lists

Comments

@wmertens
Copy link

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

@wmertens
Copy link
Author

Or maybe one for UL and one for OL

@brendancarney
Copy link
Collaborator

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

@brendancarney brendancarney added the enhancement New feature or request label Mar 20, 2019
@wmertens
Copy link
Author

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.

@brendancarney
Copy link
Collaborator

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.

@wmertens
Copy link
Author

wmertens commented Mar 21, 2019

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 :)

@brendancarney
Copy link
Collaborator

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.

@wmertens
Copy link
Author

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.

@brendancarney
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request slate-lists
Projects
None yet
Development

No branches or pull requests

2 participants