Skip to content
This repository has been archived by the owner on Feb 21, 2019. It is now read-only.

Compatible for Slate-0.32 #59

Merged
merged 25 commits into from
Feb 5, 2018
Merged

Conversation

zhujinxuan
Copy link
Contributor

In Slate-0.32, the kind is renamed to object. This plugin renames the kind to object.

Sorry for the auto merge failed; There is a small conflict in package.json by my mistake.

@SamyPesse SamyPesse requested a review from Soreine January 8, 2018 20:04
Copy link
Contributor

@Soreine Soreine left a comment

Choose a reason for hiding this comment

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

Thank you for this @zhujinxuan
Let's remove the unnecessary changes to the package.json dev dependency and it's good

"mocha": "^3.0.1",
"prettier": "^1.9.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

prettier and others should be a dependency of eslint-config-gitbook already
We will just keep the dev dependencies as they were (except for slate, slate-hyperscript etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prettier is a devDependency and peerDependency of the eslint-config-gitbook, and then it is not installed in npm i (not sure how yarn does). About eslint-plugin-prettier, eslint-config-gitbook is dependent on eslint-config-prettier rather than eslint-plugin-prettier, and eslint-plugin-prettier is the peer dependency of eslint-config-gitbook

The npm has annoying problems about different types of dependency...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two options: change the pakcage.json of the eslint-config-gitbook to add them as dependency, or to change the package.json of slate-edit-table and slate-edit-list.

Which one do you think it would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated about yarn: yarn install --peer cannot automatically install eslint, prettier and other packages (which are peer dependencies of eslint-config-gitbook) if these packages are removed from this package.json.

@jasonphillips
Copy link

Just curious as to the different approach here: but my forked library, slate-deep-table -- which has diverged quite a bit more as I updated it for 0.32 separately -- is now successfully doing almost all of the validation through the schema option (defining parents / nodes per block type), with only the special case of checking for a consistent number of columns handled on the validateNode callback.

See it here:

https://github.com/jasonphillips/slate-deep-table/blob/master/lib/makeSchema.js

I don't know what the real performance difference is, but it seemed to me to be cleaner and more canonical to express most of these validations (must contain, must be within, etc) by using the schema syntax, and only get into the more fine-grained validateNode logic for checking the table's column matching, since that requires a deeper examination.

At first I was running into the dreaded repeated validations issue that is still ongoing in slate, but with writing them carefully (and keeping normalize: false everywhere) it works well. Curious as to whether you're avoiding the schema for a reason.

@zhujinxuan
Copy link
Contributor Author

zhujinxuan commented Jan 24, 2018

@jasonphillips I would prefer the normalization by schema. I am not sure why slate-edit-table is using validateNode. This is my guess about why slate-edit-table is using validateNode:

  1. parent_object_invalid is calculated prior to child_object_invalid; Then the slate will drop off the blocks in cell by default. (Consider a quote block in document schema that quote: parent<-document)

  2. There is a potential bug about current stage of Slate that Continue normalize next child when and only when the node exists still ianstormtaylor/slate#1538. It means we cannot delete row nodes by cell normalization. I means a set of {normalize: false} everywhere.

If @Soreine appreciate it, I will submit a PR replace some validateNode with normalize after merging this PR.

Copy link
Contributor Author

@zhujinxuan zhujinxuan left a comment

Choose a reason for hiding this comment

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

@jasonphillips BTW, just wondering how do you install the peer dependency of prettier, eslint-config-prettier during the development. I tried yarn install and yarn install --peer , and both are unable to get prettier in local node_modules

"mocha": "^3.0.1",
"prettier": "^1.9.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated about yarn: yarn install --peer cannot automatically install eslint, prettier and other packages (which are peer dependencies of eslint-config-gitbook) if these packages are removed from this package.json.

@zhujinxuan
Copy link
Contributor Author

zhujinxuan commented Jan 25, 2018

Remove the prettier packages in the most recent commit

@Soreine
Copy link
Contributor

Soreine commented Jan 25, 2018

Concerning the schema, we chose to keep the old way of doing because of current bugs, just as a safety measure.
See #47 (comment) where I had converted most rules (but the column counting) to the new schema, but reverted it before publishing.

@ianstormtaylor
Copy link

Any update on this? I'd love to use this PR in production.

@zhujinxuan
Copy link
Contributor Author

@Soreine Could you merge this PR as this PR has fixed the devDependency?
I am thinking about releasing a copy-paste plugin and nested table plugin next week about refining the deleteAtRange, insertAtRange, getAtRange. But the demo and the test may need a 0.32 supported edit-table.

Thank you.

@zhujinxuan zhujinxuan force-pushed the slate-0.32-submit branch 2 times, most recently from f36ba5f to a279282 Compare February 1, 2018 22:19
@zhujinxuan
Copy link
Contributor Author

Recent commits in PR updates the babel config according to the recent changes in Slate
ianstormtaylor/slate#1557

@Soreine
Copy link
Contributor

Soreine commented Feb 5, 2018

Sorry, I understand now. I'm keeping the extra dev-dependencies. It was working on my end because I had some projects that was using them in my PATH.

@Soreine Soreine merged commit cf9c950 into GitbookIO:master Feb 5, 2018
@Soreine
Copy link
Contributor

Soreine commented Feb 5, 2018

Sorry for the delay, thanks a lot everyone!

@zhujinxuan
Copy link
Contributor Author

@Soreine I would recommend to remove the dev-dependency and add these packages to the dependency of the eslint-config-gitbook. I think it is more convenient for future updates.

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

Successfully merging this pull request may close these issues.

4 participants