Skip to content

Fix handling of redundant block config #11

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amannn
Copy link

@amannn amannn commented May 17, 2017

The last published version of this library introduced a breaking change for my app. I had blockquote specified in breakoutBlocks and the new version also has it as a default in doubleBreakoutBlocks. If this is the case, pressing enter in such a block causes the plugin to delete that line.

This PR does two things for that scenario:

  1. Print a warning.
  2. Treat the block as a regular breakout block. This way the existing behaviour doesn't break.

Is there a specific reason blockquote is specified in doubleBreakoutBlocks? If not, I'd move it to breakoutBlocks as this could be a better default. E.g. medium.com also has this behaviour.

@Rosey
Copy link
Contributor

Rosey commented Jun 14, 2017

Is there a specific reason blockquote is specified in doubleBreakoutBlocks? If not, I'd move it to breakoutBlocks as this could be a better default. E.g. medium.com also has this behaviour.

It's a taste issue of course but personally I much prefer it as double. You're right that medium doesn't do it this way but dropbox paper does, as well as the tumblr text editor... not sure what other editors but I'm sure there are more! So IMO medium is the one in the wrong here 😛 It seems common to want to do a large blockquote of multiple paragraphs and it seems annoying to have to keep "fixing" when it escapes too early.

@danneu
Copy link

danneu commented Jun 15, 2017

Agreed. It seems sensible to me that you should be able to insert newlines into blocks with multiline content like code-block and blockquote.

An alternative would be to escape after a single return yet make shift+return insert soft newline, though unfortunately that's less obvious for users as a default and doesn't work on mobile.

@makenosound
Copy link
Member

It's a taste issue of course but personally I much prefer it as double.

Yeah, this is my feeling too. Blockquotes for me are multi-line things and so it’s much easier for users to have to double-enter to break out rather than have to go back and change a block to a block quote again to achieve multiple lines.

@amannn I do like the idea of handling this more gracefully though, we certainly shouldn’t allow a block to be specified in both sets. Could you adjust the PR to include the warning only?

There’s a general problem at the moment I feel in that you have to respecify the entire set if you want something other than the defaults. I’m wondering if we should add some method of augmenting the defaults — in the past with similar configurations I’ve allowed for without: [ ... ] sort of configurations, but it gets rather messy.

Would making the defaults available as an export be enough of an interface here? Would mean you could do something like:

import createBlockBreakoutPlugin, {defaultDoubles, defaultSingles} from "draft-js-block-breakout-plugin"

const options = {
  breakoutBlockType: "unordered-list-item",
  breakoutBlocks: defaultSingles.concat(["blockquote"]),
  doubleBreakoutBlocks: defaultDoubles.filter(b => b !== "blockquote")
}
const blockBreakoutPlugin = createBlockBreakoutPlugin(options)

Thoughts?

@amannn
Copy link
Author

amannn commented Jun 19, 2017

Alright, let's keep the blockquote as a doubleBreakoutBlock then.

@makenosound The PR currently only handles the case better when a block is specified both in breakoutBlocks and doubleBreakoutBlocks. The current behaviour in master is that the block is completely removed when the user presses enter. I guess this is a bug anyway, right? Doesn't it make sense to handle this case in a more forgiving way?

I think your proposed solution to extending the defaults looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants