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

[FEATURE] [BUGFIX] Refactor editor hooks #357

Merged
merged 1 commit into from
Apr 12, 2016

Conversation

bantic
Copy link
Collaborator

@bantic bantic commented Apr 7, 2016

  • Adds postDidChange hook -- called when post changes
  • [BUGFIX BREAKING] Changes cursorDidChange to only fire when the cursor
    changes (not when markup or section state changes)
  • [FEATURE] Add inputModeDidChange hook -- called when editor input
    mode (active markups or active sections) changes

Fixes #319.

BREAKING CHANGE: This is a potentially breaking change for consumers (such as
ember-mobiledoc-editor) that used the cursorDidChange hook to
maintain toolbar state.

Most of the time the editor's input mode (active
markups and active section tagNames) changes it is due to changing
cursor position/selection, so listening to the cursorDidChange is
often appropriate, but it is possible to change the editor's input mode
without changing cursor position (e.g., hitting "cmd-B" to bold text).
Previously, the cursorDidChange hook fired over-eagerly, resulting in
it firing in some cases (but not all) when the cursor did not change
(but editor input mode did). So it served fairly effectively for keeping the
toolbar's buttons in appropriate active/inactive state. This PR fixes
cursorDidChange so that it is only called when the cursor position
actually changes, making it even less effective as a hook to use to
infer the editor's active markups and sections (now no input mode change
that doesn't change the cursor will fire cursorDidChange).

This PR introduces a new inputModeDidChange hook that only fires on
input mode changes. This cleanly separates the ability to listen for cursor changes
from listening for input mode changes. It is also more efficient for a consumer to use as a
hook (because most cursor changes do not change input mode at all, so
listening to that hook as a proxy for detecting input mode changes creates unnecessary work).

Also:

  • Test that removing a card or saving a new payload for it triggers
    postDidChange hook. Transitioning to edit or calling
    env.cancel do not trigger the hook.
  • Deprecates on('update') hook
  • Add private Editor#_notifyRangeChange to use to alert the editor
    of possible cursor- or state-changing activity (e.g., keyup or mousedown,
    because they might have moved the cursor).
  • Test that cursorDidChange is not fired after the editor is
    destroyed
  • Refactor internals of EditState to better track changes to
    state/cursor.

@mixonic
Copy link
Contributor

mixonic commented Apr 8, 2016

@bantic and I discussed this a bit. I'm hesitant to add a hook called stateDidChange or leak state as a concept outside of the internals. state is a very generic term in programming, and I worry it would miscommunicate the APIs intent easily. @bantic was sure to point out that he wants to be sure, regardless of the naming, that the documentation is correct and adequate.

We agreed that inputModeDidChange is a more reasonable name for the stateDidChange hook. Let's refactor toward that.

  * Adds `postDidChange` hook -- called when post changes
  * [BUGFIX BREAKING] Changes `cursorDidChange` to only fire when the cursor
    changes (not when markup or section state changes)
  * [FEATURE] Add `inputModeDidChange` hook -- called when editor input
    mode (active markups or active sections) changes

Fixes #319.

**BREAKING CHANGE**: This is a potentially breaking change for consumers (such as
`ember-mobiledoc-editor`) that used the `cursorDidChange` hook to
maintain toolbar state.

Most of the time the editor's input mode (active
markups and active section tagNames) changes it is due to changing
cursor position/selection, so listening to the `cursorDidChange` is
often appropriate, but it is possible to change the editor's input mode
without changing cursor position (e.g., hitting "cmd-B" to bold text).
Previously, the `cursorDidChange` hook fired over-eagerly, resulting in
it firing in some cases (but not all) when the cursor did not change
(but editor input mode did). So it served fairly effectively for keeping the
toolbar's buttons in appropriate active/inactive state. This PR fixes
`cursorDidChange` so that it is only called when the cursor position
actually changes, making it even less effective as a hook to use to
infer the editor's active markups and sections (now *no* input mode change
that doesn't change the cursor will fire `cursorDidChange`).

This PR introduces a new `inputModeDidChange` hook that only fires on
input mode changes. This cleanly separates the ability to listen for cursor changes
from listening for input mode changes. It is also more efficient for a consumer to use as a
hook (because most cursor changes do not change input mode at all, so
listening to that hook as a proxy for detecting input mode changes creates unnecessary work).

Also:

  * Test that removing a card or saving a new payload for it triggers
    `postDidChange` hook. Transitioning to `edit` or calling
    `env.cancel` do not trigger the hook.
  * Deprecates `on('update')` hook
  * Add private `Editor#_notifyRangeChange` to use to alert the editor
    of possible cursor- or state-changing activity (e.g., keyup or mousedown,
    because they might have moved the cursor).
  * Test that `cursorDidChange` is not fired after the editor is
    destroyed
  * Refactor internals of `EditState` to better track changes to
    state/cursor.
@bantic bantic force-pushed the refactor-lifecycle-callbacks branch from 8d6bc6c to de52092 Compare April 12, 2016 14:57
@bantic
Copy link
Collaborator Author

bantic commented Apr 12, 2016

stateDidChange -> inputModeDidChange

@bantic bantic merged commit e9abf0c into master Apr 12, 2016
@bantic bantic deleted the refactor-lifecycle-callbacks branch April 12, 2016 15:26
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.

refactor editor callbacks, ensure card/atom changes go through callbacks
2 participants