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

fix bug: decorate is not called for immediate children of editor #4394

Merged
merged 4 commits into from
Aug 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/four-kangaroos-invite.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'slate-react': patch
---

fix bug where decorate is not called on immediate children of editor
2 changes: 2 additions & 0 deletions packages/slate-react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
"tiny-invariant": "1.0.6"
},
"devDependencies": {
"jsdom": "^16.6.0",
"react-test-renderer": ">=16.8.0",
"slate": "^0.63.0",
"slate-hyperscript": "^0.62.0"
},
Expand Down
114 changes: 76 additions & 38 deletions packages/slate-react/src/hooks/use-children.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react'
import { Editor, Range, Element, NodeEntry, Ancestor, Descendant } from 'slate'
import { Editor, Range, Element, Path, Ancestor, Descendant } from 'slate'

import ElementComponent from '../components/element'
import TextComponent from '../components/text'
Expand All @@ -17,6 +17,67 @@ import {
* Children.
*/

const Child = (props: {
decorations: Range[]
parent: Ancestor
path: Path
child: Descendant
isLast: boolean
renderElement?: (props: RenderElementProps) => JSX.Element
renderPlaceholder: (props: RenderPlaceholderProps) => JSX.Element
renderLeaf?: (props: RenderLeafProps) => JSX.Element
selection: Range | null
}) => {
const {
decorations,
parent,
path,
child,
isLast,
renderElement,
renderPlaceholder,
renderLeaf,
selection,
} = props
const decorate = useDecorate()
const editor = useSlateStatic()

const range = Editor.range(editor, path)
const sel = selection && Range.intersection(range, selection)
const ds = decorate([child, path])

for (const dec of decorations) {
const d = Range.intersection(dec, range)

if (d) {
ds.push(d)
}
}

if (Element.isElement(child)) {
return (
<ElementComponent
decorations={ds}
element={child}
renderElement={renderElement}
renderPlaceholder={renderPlaceholder}
renderLeaf={renderLeaf}
selection={sel}
/>
)
}
return (
<TextComponent
decorations={ds}
isLast={isLast}
parent={parent}
renderPlaceholder={renderPlaceholder}
renderLeaf={renderLeaf}
text={child}
/>
)
}

const useChildren = (props: {
decorations: Range[]
node: Ancestor
Expand All @@ -33,7 +94,6 @@ const useChildren = (props: {
renderLeaf,
selection,
} = props
const decorate = useDecorate()
const editor = useSlateStatic()
const path = ReactEditor.findPath(editor, node)
const children = []
Expand All @@ -46,43 +106,21 @@ const useChildren = (props: {
const p = path.concat(i)
const n = node.children[i] as Descendant
const key = ReactEditor.findKey(editor, n)
const range = Editor.range(editor, p)
const sel = selection && Range.intersection(range, selection)
const ds = decorate([n, p])

for (const dec of decorations) {
const d = Range.intersection(dec, range)

if (d) {
ds.push(d)
}
}

if (Element.isElement(n)) {
children.push(
<ElementComponent
decorations={ds}
element={n}
key={key.id}
renderElement={renderElement}
renderPlaceholder={renderPlaceholder}
renderLeaf={renderLeaf}
selection={sel}
/>
)
} else {
children.push(
<TextComponent
decorations={ds}
key={key.id}
isLast={isLeafBlock && i === node.children.length - 1}
parent={node}
renderPlaceholder={renderPlaceholder}
renderLeaf={renderLeaf}
text={n}
/>
)
}
children.push(
<Child
decorations={decorations}
parent={node}
path={p}
child={n}
key={key.id}
isLast={isLeafBlock && i === node.children.length - 1}
renderElement={renderElement}
renderPlaceholder={renderPlaceholder}
renderLeaf={renderLeaf}
selection={selection}
/>
)

NODE_TO_INDEX.set(n, i)
NODE_TO_PARENT.set(n, node)
Expand Down
47 changes: 46 additions & 1 deletion packages/slate-react/test/index.js
Original file line number Diff line number Diff line change
@@ -1 +1,46 @@
describe('slate-react', () => {})
import * as Slate from 'slate'
import * as SlateReact from '..'
import { JSDOM } from 'jsdom'
import React from 'react'
import TestRenderer from 'react-test-renderer'
import assert from 'assert'

describe('slate-react', () => {
describe('Editable', () => {
describe('decorate', () => {
// stub out some DOM stuff to avoid crashes
before(() => {
const jsdom = new JSDOM()
global.window = jsdom.window
global.document = jsdom.window.document
global.Document = document.constructor
Comment on lines +14 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too familiar with jsdom testing, but is there any way we could extract this global setting from this one suite, so it's done for all suites (when they exist)? Or must it be done a per-suite basis?

In fact, could you put the whole decorate test in a decorate.spec.ts or similar file? The index could just be common utilities among tests for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly use Jest which has special support for jsdom https://jestjs.io/docs/configuration#testenvironment-string. I didn't find an equivalent for Mocha but there is https://github.com/rstacruz/jsdom-global which does pretty much what I do here.

I think it could be done for the whole test file, but I didn't want to presume. I figured someone with more Mocha experience would have opinions about how to set things up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest we land this as it's an improvement all around, and then we can do a separate PR to look at extracting into a global setting.

})

const createNodeMock = () => ({
ownerDocument: global.document,
getRootNode: () => global.document,
})

it('should be called on all nodes in document', () => {
const editor = SlateReact.withReact(Slate.createEditor())
const value = [{ type: 'block', children: [{ text: '' }] }]
let count = 0
const decorate = ([node, path]) => {
count++
return []
}

const el = React.createElement(
SlateReact.Slate,
{ editor, value },
React.createElement(SlateReact.Editable, { decorate })
)

TestRenderer.create(el, { createNodeMock })

// editor, block, text
assert.strictEqual(count, 3)
})
})
})
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent, we badly needed testing for slate-react

Loading