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

Separate out isInEditor from isEditableInEditor #1027

Closed
wants to merge 10 commits into from

Conversation

oyeanuj
Copy link
Contributor

@oyeanuj oyeanuj commented Aug 25, 2017

Hey @ianstormtaylor, as per your recommendation, here is the fix for #980.

The only deviation from that is that you had suggested replacing current usage of isInEditor to isEditableInEditor everywhere except onDrop. Looking at the code, it seems like the following methods should still check for only isInEditor since these seem like valid operations on a void node.

  1. onCopy
  2. onCut
  3. onDragEnd
  4. onDragOver
  5. onDragStart
  6. onPaste

Let me know if you think otherwise.

oyeanuj and others added 8 commits January 27, 2017 20:20
* ianstormtaylor/master:
  Clarifying insertTextByKey description (ianstormtaylor#778)
  Documenting node's 'getFirstText' & 'getLastText' (ianstormtaylor#779)
  0.19.22
  fix selection handling for changing tabs, and inside embedded inputs, closes ianstormtaylor#749
  fix to restrict window blur/focus handling, closes ianstormtaylor#773
  remove warn throwing since console.warn includes callsites now
  Update defining-custom-block-nodes.md (ianstormtaylor#776)
  0.19.21
  update table example to make scope clearer
  Fixed the link to comparisons, which was broken (ianstormtaylor#769)
  fix error when dragging void nodes without selection, closes ianstormtaylor#757
  fix to depend on prop-types for react 15.5
  fix to maintain focus on switching tabs, closes ianstormtaylor#756
  update issue template
  Add "data-key" to root div for the whole document (ianstormtaylor#759)
  add an issue template
* ianstormtaylor/master: (21 commits)
  Fix json (ianstormtaylor#798)
  update readme
  0.19.29
  allow memoization without arguments for faster lookups (ianstormtaylor#796)
  0.19.28
  Add tests for plugins (ianstormtaylor#792)
  improve perf delaying conversion to immutable objects (ianstormtaylor#794)
  0.19.27
  fix schema normalizing to merge into history
  0.19.26
  `moveNode` operation fix (ianstormtaylor#782)
  fix large document example imports
  refactor examples, upgrade dependencies
  v0.19.25
  fix void copying to attach to the right dom element
  0.19.24
  0.19.23
  Add firstOnly prop to Placeholder (ianstormtaylor#765)
  rename issue template
  Copy void (ianstormtaylor#788)
  ...
* ianstormtaylor/master: (61 commits)
  0.21.2
  alphabetize package.json scripts
  Speed up getting blocks at a range
  Remove instanceOf checks to allow Slate objects to be identifiable across module instances (ianstormtaylor#930)
  Refactor render arrow functions (ianstormtaylor#969)
  update prosemirror description in readme
  update prosemirror description in docs
  update prosemirror comparison in readme
  0.21.1
  Fix for HTML pasting not working in IE (ianstormtaylor#882)
  Remove unneeded check. (ianstormtaylor#961)
  Upgrade react-frame-component. (ianstormtaylor#962)
  adding forced-layout-example (ianstormtaylor#954)
  Reckon with inconsistencies between parse5 and native DOMParser (ianstormtaylor#952)
  Omit version on gzip size badge (ianstormtaylor#947)
  update issue template and contributing docs
  add reboo editor to resources
  add showcase to resources
  0.21.0
  update changelog
  ...
* ianstormtaylor/master: (21 commits)
  0.21.3
  Add .babelrc into .npmignore (ianstormtaylor#1020)
  Add a missing dependency which was causing errors (ianstormtaylor#1017)
  Fixes ianstormtaylor#1010 — adds debuginfo to Contributing.md (ianstormtaylor#1011)
  Add State.characters to docs (ianstormtaylor#1006)
  Use parent node to find text node parent (ianstormtaylor#1004)
  IE11 compat (ianstormtaylor#996)
  Add .editorconfig and .gitattributes (ianstormtaylor#997)
  Improve documentation (ianstormtaylor#1000)
  Consistently use toLowerCase in tag name comparisons. (ianstormtaylor#1001)
  Custom wrapper component (ianstormtaylor#978)
  Add link to IME reference page in Contributing.md (ianstormtaylor#995)
  Don't break macOS emoji insert nor composition when fixing ianstormtaylor#938 (ianstormtaylor#994)
  When the `isInEditor` parameter is a text node use its parent only if the parent exists (ianstormtaylor#974)
  Document Raw serializer options (ianstormtaylor#993)
  tagName returns capitalized name now (ianstormtaylor#985)
  Fixes ianstormtaylor#938 chrome eating first line of text on space at beginning of line (ianstormtaylor#991)
  Fixes ianstormtaylor#817; fragment matcher regex (ianstormtaylor#992)
  Update ISSUE_TEMPLATE.md
  Update Contributing.md
  ...
Copy link
Owner

@ianstormtaylor ianstormtaylor left a comment

Choose a reason for hiding this comment

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

Hey @oyeanuj, sweet! Looks good. Have you tested it to be sure that the ones that are switching don't cause any issues?

* @param {Element} target
* @return {Boolean}
*/

isInEditor = (target) => {
const { element } = this
// COMPAT: Text nodes don't have `isContentEditable` property. So, when
// `target` is a text node use its parent node for check.
const el = target.nodeType === 3 ? target.parentNode : target
Copy link
Owner

Choose a reason for hiding this comment

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

I think we no longer need this line (and comment) in the isInEditor function, judging by the comments text since it was only to allow for using el.isContentEditable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@oyeanuj
Copy link
Contributor Author

oyeanuj commented Aug 25, 2017

@ianstormtaylor So, the only issue that is coming up in testing so far is the Embed example. Pasting into an input in the void node breaks unless I update the onPaste call to use isEditableInEditor as well. Right now, as expected, it pastes it where the selection currently. I can see us making the void node the selection on paste, and pasting right after.. but then that breaks the Embed example.

On the other hand, we can check for isEditableInEditor, but that feels a bit wrong since one should be able to paste on a void node? Thoughts?

I'll keep testing a little bit more to ensure no breakage.

@ianstormtaylor
Copy link
Owner

Fixed by #3093.

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

Successfully merging this pull request may close these issues.

2 participants