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

Upgrade to slate 0.44 #6

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

Conversation

gnestor
Copy link

@gnestor gnestor commented Oct 11, 2018

Some slate API changes: https://github.com/ianstormtaylor/slate/blob/master/packages/slate/Changelog.md#0420--october-9-2018

This also adds prettier for code formatting 👍

I was running into some error when running the example but I tested in my own project and it's working with slate 0.42.2.

UPDATE: I've push a new commit that upgrades to slate 0.44.

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated
toRemove.forEach((failure) => change = change.removeNodeByKey(failure.key))
return true
function onChange(change, next) {
if (!canBeEmpty) return next();

Choose a reason for hiding this comment

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

Sorry, I realize this is old code but this is a little confusing to me and is smells like a performance issue. Is this going over the entire document to look for void inlines and rip them out? What's the purpose of this event handler?

Might be a better question for @YurkaninRyan

Copy link
Owner

Choose a reason for hiding this comment

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

Hey Cameron, it's been awhile but we use zero-width spaces to keep "empty" inlines around. This cleans them up if you unfocus them.

There may be a better way to do this.

lib/index.js Outdated Show resolved Hide resolved
function onSelect(event, change, next) {
const selection = findRange(window.getSelection(), change.editor);
const selectionIsAtEndOfInline =
change.value.focusInline &&
selection.focusOffset === change.value.focusInline.text.length;

Choose a reason for hiding this comment

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

I dont think we're supposed to return the change here anymore when the plugin runs a command. I believe the correct thing to return is undefined. But I'm not exactly sure.

Copy link
Author

Choose a reason for hiding this comment

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

We only return next() if we want plugins down the stack to continue handling the event. If we want to handle the event ourselves and stop propagation, then we don't return anything (although I think it's ok to return anything else but next()).

lib/onArrowRight.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
@cameracker
Copy link

Hi, I hope you and @YurkaninRyan don't mind but I was also looking at upgrading this plugin for my own upgrade and happened upon this PR. So I took the liberty of reviewing it. The changes look good I think! Just a few things that I believe need to be corrected.
In summary:

  1. Looks like a prettier was run on the code, which is cool, but there are some lines that used to be one line conditional returns that are now broken up across multiple lines. I think because the conditional and its body are broken up across multiple lines now, the best thing to do is to wrap the body with { } to avoid problems down the line with weird merges or subtle mistakes with code.
  2. The event handler functions accept a change argument as before, but now this is a reference to the editor controller. My recommendation is to rename change in all cases to editor (except in the case that its actually a change object) because going forward this will probably be confusing to folks who might wonder why commands are being run on a change (can't do that anymore).
  3. When the handler functions run, they are returning an editor object. I'm not sure what this will do because the recommended practice for event handler functions is to have them return next() to continue along the plugin stack and to return nothing if the event was handled and the plugin stack should not be resumed. I think this might be the source of an error later, but I'm not sure.
  4. The collapseTo and extendTo selection functions were deprecated and were renamed. We should fix those functions to have the correct name.

Thanks for putting in this PR, hopefully ryan gets a chance to look at it soon :)

@gnestor gnestor changed the title Upgrade to slate 0.42 Upgrade to slate 0.44 Feb 13, 2019
@tommoor
Copy link

tommoor commented Feb 16, 2019

Hey @YurkaninRyan – thanks for the great plugin here. Are you interested in merging and publishing these updates which are required to work with the latest Slate versions or should we go ahead and fork the project?

lib/index.js Outdated Show resolved Hide resolved
@twavv
Copy link

twavv commented Mar 15, 2019

It seems pretty clear that the chances of this project being updates are low. Can we (well, you, to be specific) create a fork and publish to npm (maybe as slate-inline-selection-plugin or something similar as we can't use the same name)?

@gnestor
Copy link
Author

gnestor commented Mar 15, 2019

@travigd I can do that. I intend to create a monorepo (like slate-plugins) to share a bunch of plugins that I have built. I suppose that I should do that sooner than later so that I can publish this and others can start doing that.

Additionally, it would be nice to bring some more organization and collaboration to the slate plugin ecosystem. By creating an actively maintained monorepo, it should prevent things like this (out-of-date and unmaintained repos) from happening and make it easier for others to contribute. We could either combine forces with slate-plugins and add some plugins to it or start a new one. Open to ideas...

@twavv
Copy link

twavv commented Mar 15, 2019

Maybe an @slate-contrib npm org? I'd be happy to help (at least a bit).

@Jarred-Sumner
Copy link

Jarred-Sumner commented May 14, 2019

Is there a way to use this fork? I tried installing it and then whitelisting it in the babel config (so it gets compiled), but it doesn't seem to be doing anything

@gnestor
Copy link
Author

gnestor commented May 14, 2019

Hi @Jarred-Sumner, I've been meaning to create a monorepo or something that is more contributor-friendly but I haven't gotten around to it. In the meantime, you should be able to clone my fork, check out the slate-0.42 branch, install, and then yarn link and then go to your project that depends on slate-sticky-inlines and then run yarn link slate-sticky-inlines.

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.

6 participants