facebookarchive / draft-js

A React framework for building text editors.
https://draftjs.org/
MIT License
22.57k stars 2.64k forks source link

onTab only works with ordered/unordered list block types #1773

Open alexlatchford opened 6 years ago

alexlatchford commented 6 years ago

Do you want to request a feature or report a bug?

I guess a feature request, more like a minor behaviour change.

In short: onTab is hard-coded on only work with unordered-list-item & ordered-list-item block types. I'd like to change the API to allow for that set to be customised.

What is the current behavior?

https://github.com/facebook/draft-js/blob/master/src/model/modifier/RichTextEditorUtil.js#L207

Currently RichUtils.onTab limited to only work with unordered-list-item & ordered-list-item block types.

What is the expected behavior?

I'd like to canvas opinion on adding an acceptedTypes = ['unordered-list-item', 'ordered-list-item'] argument to onTab.

Why? I'm adding support for checklists (similar to Githubs) which are implemented using a custom block renderer component. Surprisingly if you use the Github markdown syntax - [ ] it mostly works, it needs a a bit of styling tweaks.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?

All as far as I can tell. No blaming the line this statement has been there since the project was open sourced.


Happy to submit a PR but wanted to gauge opinion first before committing. Will try to find another way around the problem if not.

PS. Great project we make huge use of it at Hugo!

andrewbranch commented 6 years ago

@alexlatchford tangentially related and perhaps interesting to you: I found myself wanting to support key commands other than Tab and Shift+Tab for changing the depth of list items, but RichUtils.onTab (as the name implies) couples (at least from the consumer’s perspective) its logic of a) figuring out from the event what you meant to do, b) figuring out from the current content whether a depth adjustment is possible, and c) actually performing the depth adjustment. (In actual fact, only a and b are totally coupled, and c is done via adjustBlockDepthForContentState.)

I was considering proposing adding additional functions to RichUtils called increaseDepth() and decreaseDepth() that would work the same as onTab, except not need an event parameter, so they could be called from handleKeyCommand, for example.

I can potentially see your proposal and mine going hand-in-hand. Or, in the meantime, you may be able to get pretty far just by importing adjustBlockDepthForContentState and using it directly.

Thoughts?

alexlatchford commented 6 years ago

@andrewbranch sounds good to me. It certainly makes sense to de-couple depth adjustment from invoking via tab so would get my vote.

niveditc commented 6 years ago

@alexlatchford, @andrewbranch - I'm currently implementing onTab behaviors for the new exploratory tree data features (currently implemented in NestedRichTextEditorUtil.js). Running into this consideration there as well, so I'll think about it a bit & hopefully add this flexibility to both APIs. Might take a few weeks though :)

andrewbranch commented 6 years ago

Cool! Given the discussion in #1193 and my own experience in #1811, I didn’t think this project was really accepting any non-trivial changes at this point. Good to hear though!

niveditc commented 6 years ago

@andrewbranch - we're switching to new maintainers internally and I'm one of them. It's still a side project for us and we're leaning on the safe side to make sure that any PRs pulled in don't break the project + production Facebook (which uses it heavily).

We had a meeting on Friday, and I will be posting meeting notes on Monday that will hopefully give the community a bit more clarity. I'll also post a link to those on the discussion in #1193.

I'm probably not going to get a chance to look into #1811 until next weekend at the earliest since I'm signing off for today & will probably need to test that PR on our stack before merging :)