ronantakizawa/codereview-bench huggingface dataset:
Repository: ianstormtaylor/slate
File: packages/slate-react/src/components/leaf.tsx (TypeScript)

Code before:

    return () => {
      editorEl.style.minHeight = 'auto'
    }
  }, [placeholderRef, leaf])

  let children = (
    <String isLast={isLast} leaf={leaf} parent={parent} text={text} />
  )

  if (leaf[PLACEHOLDER_SYMBOL]) {
    const placeholderProps = {
      ref: placeholderRef,
      contentEditable: false,
      style: {
        position: 'absolute',
        pointerEvents: 'none',
        width: '100%',
        maxWidth: '100%',
        display: 'block',
        opacity: '0.333',
        userSelect: 'none',
        fontStyle: 'normal',
        fontWeight: 'normal',
        textDecoration: 'none',
      } as React.CSSProperties,
    }

    children = (
      <React.Fragment>
        {typeof leaf.placeholder === 'string'
          ? defaultRenderPlaceholder({
              ...placeholderProps,
              children: leaf.placeholder,
            })
          : leaf.placeholder(placeholderProps)}
        {children}
      </React.Fragment>
    )
  }

  // COMPAT: Having the `data-` attributes on these leaf elements ensures that
  // in certain misbehaving browsers they aren't weirdly cloned/destroyed by
  // contenteditable behaviors. (2019/05/08)
  const attributes: {
    'data-slate-leaf': true
  } = {
    'data-slate-leaf': true,
  }

  return renderLeaf({ attributes, children, leaf, text })

Review comment (suggestion):
And then all this placeholder rendering logic can move out of the `<Leaf>` and be consolidated into the top-level `<Editable>`. 

The only props we'd be passing in to the `renderPlaceholder` would be `children` (which is the `placeholder` prop from the editor) and `ref`. The other things here would just be defined inline in our new `<DefaultPlaceholder>` component.

Code after:
    />
  )

  if (leaf[PLACEHOLDER_SYMBOL]) {
    const placeholderProps: RenderPlaceholderProps = {
      children: leaf.placeholder,
      attributes: {
        'data-slate-placeholder': true,
        style: {
          position: 'absolute',
          pointerEvents: 'none',
          width: '100%',
          maxWidth: '100%',
          display: 'block',
          opacity: '0.333',
          userSelect: 'none',
          textDecoration: 'none',
        },
        contentEditable: false,
        ref: placeholderRef,
      },
    }

    children = (
      <React.Fragment>
        {renderPlaceholder(placeholderProps)}
        {children}
      </React.Fragment>
    )
  }

  // COMPAT: Having the `data-` attributes on these leaf elements ensures that
  // in certain misbehaving browsers they aren't weirdly cloned/destroyed by
  // contenteditable behaviors. (2019/05/08)
  const attributes: {
    'data-slate-leaf': true
  } = {
    'data-slate-leaf': true,
  }

  return renderLeaf({ attributes, children, leaf, text })
}

const MemoizedLeaf = React.memo(Leaf, (prev, next) => {
  return (
    next.parent === prev.parent &&
    next.isLast === prev.isLast &&
    next.renderLeaf === prev.renderLeaf &&
    next.renderPlaceholder === prev.renderPlaceholder &&
    next.text === prev.text &&
    next.leaf.text === prev.leaf.text &&
Repository: ianstormtaylor/slate
File: packages/slate-react/src/components/editable.tsx (TypeScript)

Code before:
  contentEditable: boolean
  children?: React.ReactNode
  ref: React.RefObject<any>
}

/**
 * Default placeholder style object.
 */
export const defaultPlaceholderStyle: React.CSSProperties = {
  position: 'absolute',
  pointerEvents: 'none',
  width: '100%',
  maxWidth: '100%',
  display: 'block',
  opacity: '0.333',
  userSelect: 'none',
  textDecoration: 'none',
}

/**
 * The default placeholder render method
 */

export const defaultRenderPlaceholder = (props: RenderPlaceholderProps) => (
  <span style={defaultPlaceholderStyle} {...props} />
)

/**
 * A default memoized decorate function.
 */

const defaultDecorate: (entry: NodeEntry) => Range[] = () => []

/**
 * Check if two DOM range objects are equal.
 */

const isRangeEqual = (a: DOMRange, b: DOMRange) => {
  return (
    (a.startContainer === b.startContainer &&
      a.startOffset === b.startOffset &&
      a.endContainer === b.endContainer &&
      a.endOffset === b.endOffset) ||
    (a.startContainer === b.endContainer &&
      a.startOffset === b.endOffset &&
      a.endContainer === b.startContainer &&
      a.endOffset === b.startOffset)
  )
}

/**

Review comment (suggestion):
I see what you mean about needing `forwardRef` to keep the resize logic. Is there any reason `forwardRef` wasn't working? It doesn't seem bad in my opinion to have a `<DefaultPlaceholder>` component that has ref-forwarding built in. And would stay consistent that way.

And then that `DefaultPlaceholder` component can be exported (like the others), and this little rendering function can stay private.

Code after:
}

/**
 * The props that get passed to renderPlaceholder
 */
export type RenderPlaceholderProps = {
  children: any
  attributes: {
    'data-slate-placeholder': boolean
    dir?: 'rtl'
    contentEditable: boolean
    ref: React.RefObject<any>
    style: React.CSSProperties
  }
}

/**
 * The default placeholder element
 */

export const DefaultPlaceholder = ({
  attributes,
  children,
}: RenderPlaceholderProps) => <span {...attributes}>{children}</span>

/**
 * A default memoized decorate function.
 */

const defaultDecorate: (entry: NodeEntry) => Range[] = () => []

/**
 * Check if two DOM range objects are equal.
 */

const isRangeEqual = (a: DOMRange, b: DOMRange) => {
  return (
    (a.startContainer === b.startContainer &&
      a.startOffset === b.startOffset &&
      a.endContainer === b.endContainer &&
      a.endOffset === b.endOffset) ||
    (a.startContainer === b.endContainer &&
      a.startOffset === b.endOffset &&
      a.endContainer === b.startContainer &&
      a.endOffset === b.startOffset)
  )
}

/**
 * Check if the target is in the editor.
 */
Repository: ianstormtaylor/slate
File: packages/slate-react/src/components/editable.tsx (TypeScript)

Code before:
  decorate?: (entry: NodeEntry) => Range[]
  onDOMBeforeInput?: (event: InputEvent) => void
  placeholder?: string
  readOnly?: boolean
  role?: string
  style?: React.CSSProperties
  renderElement?: (props: RenderElementProps) => JSX.Element
  renderLeaf?: (props: RenderLeafProps) => JSX.Element
  renderPlaceholder?: (props: RenderPlaceholderProps) => JSX.Element
  as?: React.ElementType
} & React.TextareaHTMLAttributes<HTMLDivElement>

/**
 * Editable.
 */

export const Editable = (props: EditableProps) => {
  const {
    autoFocus,
    decorate = defaultDecorate,
    onDOMBeforeInput: propsOnDOMBeforeInput,
    placeholder,
    readOnly = false,
    renderElement,
    renderLeaf,
    renderPlaceholder = DefaultPlaceholder,
    style = {},
    as: Component = 'div',
    ...attributes
  } = props
  const editor = useSlate()
  const ref = useRef<HTMLDivElement>(null)

  // Update internal state on each render.
  IS_READ_ONLY.set(editor, readOnly)

  // Keep track of some state for the event handler logic.
  const state = useMemo(
    () => ({
      isComposing: false,
      isUpdatingSelection: false,
      latestElement: null as DOMElement | null,
    }),
    []
  )

  // Update element-related weak maps with the DOM element ref.
  useIsomorphicLayoutEffect(() => {
    let window
    if (ref.current && (window = getDefaultView(ref.current))) {
      EDITOR_TO_WINDOW.set(editor, window)

Review comment (suggestion):
This shouldn't be just the component as default but instead a function that returns the component like:

```tsx
(props) => <DefaultPlaceholder {...props} />
```

Code after:
  decorate?: (entry: NodeEntry) => Range[]
  onDOMBeforeInput?: (event: InputEvent) => void
  placeholder?: string
  readOnly?: boolean
  role?: string
  style?: React.CSSProperties
  renderElement?: (props: RenderElementProps) => JSX.Element
  renderLeaf?: (props: RenderLeafProps) => JSX.Element
  renderPlaceholder?: (props: RenderPlaceholderProps) => JSX.Element
  as?: React.ElementType
} & React.TextareaHTMLAttributes<HTMLDivElement>

/**
 * Editable.
 */

export const Editable = (props: EditableProps) => {
  const {
    autoFocus,
    decorate = defaultDecorate,
    onDOMBeforeInput: propsOnDOMBeforeInput,
    placeholder,
    readOnly = false,
    renderElement,
    renderLeaf,
    renderPlaceholder = props => <DefaultPlaceholder {...props} />,
    style = {},
    as: Component = 'div',
    ...attributes
  } = props
  const editor = useSlate()
  const ref = useRef<HTMLDivElement>(null)

  // Update internal state on each render.
  IS_READ_ONLY.set(editor, readOnly)

  // Keep track of some state for the event handler logic.
  const state = useMemo(
    () => ({
      isComposing: false,
      isUpdatingSelection: false,
      latestElement: null as DOMElement | null,
    }),
    []
  )

  // Update element-related weak maps with the DOM element ref.
  useIsomorphicLayoutEffect(() => {
    let window
    if (ref.current && (window = getDefaultView(ref.current))) {
      EDITOR_TO_WINDOW.set(editor, window)
Repository: ianstormtaylor/slate
File: packages/slate/src/utils/deep-equal.ts (TypeScript)

Code before:
import isPlainObject from 'is-plain-object'

/*
  Custom deep equal comparison for Slate nodes.

  We don't need general purpose deep equality;
  Slate only suports plain values and nested objects.

  Slate objects are designed to be serialised, so
  missing keys are deliberately normalised to undefined.
 */
export const isDeepEqual = (
  text: Record<string, any>,
  another: Record<string, any>
): boolean => {
  for (const key in text) {
    if (isPlainObject(text[key])) {
      return isDeepEqual(text[key], another[key])
    } else if (text[key] !== another[key]) {
      return false
    }
  }

  /*
    Deep object equality is only necessary in one direction.

    In the reverse direction we are only looking for missing keys.
  */

  for (const key in another) {
    if (text[key] !== another[key]) {
      return false

Review comment (suggestion):
Generally very supportive of this change, just one question:

Is this limitation documented somewhere? I only quickly looked at the docs, but didn't find anything relevant other than:

> Text nodes too can contain any custom properties you want

At the moment Slate supports merging of text nodes that have identical arrays, for example, these text nodes would be merged today.

```ts
{
  text: 'Hello',
  marks: ['bold, 'italic'],
},
{
  text: ' world',
  marks: ['bold, 'italic'],
}
```

Your change would be a breaking change for consumers that do this, which is why I'm wondering if we ever documented this.

Code after:
import isPlainObject from 'is-plain-object'

/*
  Custom deep equal comparison for Slate nodes.

  We don't need general purpose deep equality;
  Slate only supports plain values, Arrays, and nested objects.
  Complex values nested inside Arrays are not supported.

  Slate objects are designed to be serialised, so
  missing keys are deliberately normalised to undefined.
 */
export const isDeepEqual = (
  node: Record<string, any>,
  another: Record<string, any>
): boolean => {
  for (const key in node) {
    const a = node[key]
    const b = another[key]
    if (isPlainObject(a)) {
      if (!isDeepEqual(a, b)) return false
    } else if (Array.isArray(a) && Array.isArray(b)) {
      if (a.length !== b.length) return false
      for (let i = 0; i < a.length; i++) {
        if (a[i] !== b[i]) return false
      }
      return true
    } else if (a !== b) {
      return false
    }
  }
Repository: ianstormtaylor/slate
File: packages/slate-react/src/components/editable.tsx (TypeScript)

Code before:
  return Editor.isVoid(editor, slateNode)
}

/**
 * Check if an event is overrided by a handler.
 */

export const isEventHandled = <
  EventType extends React.SyntheticEvent<unknown, unknown>
>(
  event: EventType,
  handler?: (event: EventType) => void | boolean
) => {
  if (!handler) {
    return false
  }
  // The custom event handler may return a boolean to specify whether the event
  // shall be treated as being handled or not.
  const shouldTreatEventAsHandled = handler(event)

  return (
    (shouldTreatEventAsHandled === true ||
      event.isDefaultPrevented() ||
      event.isPropagationStopped()) &&
    shouldTreatEventAsHandled !== false
  )
}

/**
 * Check if a DOM event is overrided by a handler.
 */

export const isDOMEventHandled = <E extends Event>(
  event: E,
  handler?: (event: E) => void
) => {
  if (!handler) {
    return false
  }

  handler(event)
  return event.defaultPrevented
}


Review comment (suggestion):
This code is a bit hard to follow, could we break it down?
```suggestion
  if (shouldTreatEventAsHandled != null) {
    return shouldTreatEventAsHandled
  }
  
  return event.isDefaultPrevented() || event.isPropagationStopped()
```



Code after:
  return Editor.isVoid(editor, slateNode)
}

/**
 * Check if an event is overrided by a handler.
 */

export const isEventHandled = <
  EventType extends React.SyntheticEvent<unknown, unknown>
>(
  event: EventType,
  handler?: (event: EventType) => void | boolean
) => {
  if (!handler) {
    return false
  }
  // The custom event handler may return a boolean to specify whether the event
  // shall be treated as being handled or not.
  const shouldTreatEventAsHandled = handler(event)

  if (shouldTreatEventAsHandled != null) {
    return shouldTreatEventAsHandled
  }

  return event.isDefaultPrevented() || event.isPropagationStopped()
}

/**
 * Check if a DOM event is overrided by a handler.
 */

export const isDOMEventHandled = <E extends Event>(
  event: E,
  handler?: (event: E) => void | boolean
) => {
  if (!handler) {
    return false
  }

  // The custom event handler may return a boolean to specify whether the event
  // shall be treated as being handled or not.
  const shouldTreatEventAsHandled = handler(event)

  if (shouldTreatEventAsHandled != null) {
    return shouldTreatEventAsHandled
  }

  return event.defaultPrevented
}
Repository: ianstormtaylor/slate
File: packages/slate/src/interfaces/element.ts (TypeScript)

Code before:
import isPlainObject from 'is-plain-object'
import { Editor, Node, Path, Descendant, ExtendedType, Ancestor } from '..'

/**
 * `Element` objects are a type of node in a Slate document that contain other
 * element nodes or text nodes. They can be either "blocks" or "inlines"
 * depending on the Slate editor's configuration.
 */

export interface BaseElement {
  children: Descendant[]
}

export type Element = ExtendedType<'Element', BaseElement>

export interface ElementInterface {
  isAncestor: (value: any) => value is Ancestor
  isElement: (value: any) => value is Element
  isElementList: (value: any) => value is Element[]
  isElementProps: (props: any) => props is Partial<Element>
  isElementType: (
    value: any,
    type: string
  ) => value is Element & { type: string }
  matches: (element: Element, props: Partial<Element>) => boolean
}

/**
 * Shared the function with isElementType utility
 */
const isElement = (value: any): value is Element => {
  return (
    isPlainObject(value) &&
    Node.isNodeList(value.children) &&
    !Editor.isEditor(value)
  )
}

export const Element: ElementInterface = {
  /**
   * Check if a value implements the 'Ancestor' interface.
   */

  isAncestor(value: any): value is Ancestor {
    return isPlainObject(value) && Node.isNodeList(value.children)
  },

  /**
   * Check if a value implements the `Element` interface.

Review comment (suggestion):
I would suggest maybe using a Generic type for this:

```suggestion
  isElementType: <T extends Element>(
    value: any,
    discriminantVal: string,
    discriminantKey?: string
  ) => value is T
```

Code after:
import isPlainObject from 'is-plain-object'
import { Editor, Node, Path, Descendant, ExtendedType, Ancestor } from '..'

/**
 * `Element` objects are a type of node in a Slate document that contain other
 * element nodes or text nodes. They can be either "blocks" or "inlines"
 * depending on the Slate editor's configuration.
 */

export interface BaseElement {
  children: Descendant[]
}

export type Element = ExtendedType<'Element', BaseElement>

export interface ElementInterface {
  isAncestor: (value: any) => value is Ancestor
  isElement: (value: any) => value is Element
  isElementList: (value: any) => value is Element[]
  isElementProps: (props: any) => props is Partial<Element>
  isElementType: <T extends Element>(
    value: any,
    elementVal: string,
    elementKey?: string
  ) => value is T
  matches: (element: Element, props: Partial<Element>) => boolean
}

/**
 * Shared the function with isElementType utility
 */
const isElement = (value: any): value is Element => {
  return (
    isPlainObject(value) &&
    Node.isNodeList(value.children) &&
    !Editor.isEditor(value)
  )
}

export const Element: ElementInterface = {
  /**
   * Check if a value implements the 'Ancestor' interface.
   */

  isAncestor(value: any): value is Ancestor {
    return isPlainObject(value) && Node.isNodeList(value.children)
  },

  /**
Repository: ianstormtaylor/slate
File: packages/slate/src/interfaces/element.ts (TypeScript)

Code before:

  isElementList(value: any): value is Element[] {
    return Array.isArray(value) && value.every(val => Element.isElement(val))
  },

  /**
   * Check if a set of props is a partial of Element.
   */

  isElementProps(props: any): props is Partial<Element> {
    return (props as Partial<Element>).children !== undefined
  },

  /**
   * Check if a value implements the `Element` interface and has type key
   */

  isElementType: (
    value: any,
    type: string
  ): value is Element & { type: string } => {
    if (value?.type === type && isElement(value)) {
      return true
    }
    return false
  },

  /**
   * Check if an element matches set of properties.
   *
   * Note: this checks custom properties, and it does not ensure that any
   * children are equivalent.
   */

  matches(element: Element, props: Partial<Element>): boolean {
    for (const key in props) {
      if (key === 'children') {
        continue
      }

      if (element[key] !== props[key]) {
        return false
      }
    }

    return true
  },
}

/**
 * `ElementEntry` objects refer to an `Element` and the `Path` where it can be

Review comment (suggestion):
Maybe do something like the below to make the func more flexible?

```suggestion
  isElementType: (
    value: any,
    discriminantVal: string,
    discriminantKey: string = 'type'
  ): value is Element & { type: string } => {
    return isElement(value) && value[discriminantKey] === discriminantVal;
  },
```

Code after:
   */

  isElementList(value: any): value is Element[] {
    return Array.isArray(value) && value.every(val => Element.isElement(val))
  },

  /**
   * Check if a set of props is a partial of Element.
   */

  isElementProps(props: any): props is Partial<Element> {
    return (props as Partial<Element>).children !== undefined
  },

  /**
   * Check if a value implements the `Element` interface and has elementKey with selected value.
   * Default it check to `type` key value
   */

  isElementType: <T extends Element>(
    value: any,
    elementVal: string,
    elementKey: string = 'type'
  ): value is T => {
    return isElement(value) && value[elementKey] === elementVal
  },

  /**
   * Check if an element matches set of properties.
   *
   * Note: this checks custom properties, and it does not ensure that any
   * children are equivalent.
   */

  matches(element: Element, props: Partial<Element>): boolean {
    for (const key in props) {
      if (key === 'children') {
        continue
      }

      if (element[key] !== props[key]) {
        return false
      }
    }

    return true
  },
}

/**
 * `ElementEntry` objects refer to an `Element` and the `Path` where it can be
Repository: ianstormtaylor/slate
File: site/examples/mentions.tsx (TypeScript)

Code before:
    type: 'mention',
    character,
    children: [{ text: '' }],
  }
  Transforms.insertNodes(editor, mention)
  Transforms.move(editor)
}

const Element = props => {
  const { attributes, children, element } = props
  switch (element.type) {
    case 'mention':
      return <Mention {...props} />
    default:
      return <p {...attributes}>{children}</p>
  }
}

const Mention = ({ attributes, children, element }) => {
  const selected = useSelected()
  const focused = useFocused()
  return (
    <span
      {...attributes}
      contentEditable={false}
      data-test={`mention-${element.character.replace(' ', '-')}`}
      style={{
        padding: '3px 3px 2px',
        margin: '0 1px',
        verticalAlign: 'baseline',
        display: 'inline-block',
        borderRadius: '4px',
        backgroundColor: '#eee',
        fontSize: '0.9em',
        boxShadow: selected && focused ? '0 0 0 2px #B4D5FF' : 'none',
      }}
    >
      @{element.character}
      {children}
    </span>
  )
}

const initialValue: Descendant[] = [
  {
    type: 'paragraph',
    children: [
      {
        text:
          'This example shows how you might implement a simple @-mentions feature that lets users autocomplete mentioning a user by their username. Which, in this case means Star Wars characters. The mentions are rendered as void inline elements inside the document.',
      },

Review comment (suggestion):
`data-cy` might be a better name for these tags, as its only use is to be used by cypress tests specifically
```suggestion
      data-cy={`mention-${element.character.replace(' ', '-')}`}
```
`data-cy` is also the preferred name used in Cypress Best Practices

Code after:
    type: 'mention',
    character,
    children: [{ text: '' }],
  }
  Transforms.insertNodes(editor, mention)
  Transforms.move(editor)
}

const Element = props => {
  const { attributes, children, element } = props
  switch (element.type) {
    case 'mention':
      return <Mention {...props} />
    default:
      return <p {...attributes}>{children}</p>
  }
}

const Mention = ({ attributes, children, element }) => {
  const selected = useSelected()
  const focused = useFocused()
  return (
    <span
      {...attributes}
      contentEditable={false}
      data-cy={`mention-${element.character.replace(' ', '-')}`}
      style={{
        padding: '3px 3px 2px',
        margin: '0 1px',
        verticalAlign: 'baseline',
        display: 'inline-block',
        borderRadius: '4px',
        backgroundColor: '#eee',
        fontSize: '0.9em',
        boxShadow: selected && focused ? '0 0 0 2px #B4D5FF' : 'none',
      }}
    >
      @{element.character}
      {children}
    </span>
  )
}

const initialValue: Descendant[] = [
  {
    type: 'paragraph',
    children: [
      {
        text:
          'This example shows how you might implement a simple @-mentions feature that lets users autocomplete mentioning a user by their username. Which, in this case means Star Wars characters. The mentions are rendered as void inline elements inside the document.',
      },