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.

Sign up for GitHub

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

Jump to bottom

refactor editor callbacks, ensure card/atom changes go through callbacks #319

Closed
5 tasks done
bantic opened this issue Feb 9, 2016 · 0 comments · Fixed by #357
Closed
5 tasks done

refactor editor callbacks, ensure card/atom changes go through callbacks #319

bantic opened this issue Feb 9, 2016 · 0 comments · Fixed by #357
Assignees
@bantic

Comments

@bantic
Copy link
Collaborator

bantic commented Feb 9, 2016

Refactor and document the editor's lifecycle callbacks.

  • Remove the on event-emitting construction in favor of registering callbacks by name
  • Ensure changes to card payload trigger the appropriate callbacks
  • Ensure changing card display state triggers callbacks
  • deprecate on('update') (aka didUpdate) callback (in favor of didRender)
  • do not run callbacks after editor is destroyed
@bantic bantic self-assigned this Feb 11, 2016
bantic added a commit that referenced this issue Apr 5, 2016
@bantic
[CLEANUP] Refactor lifecycle callbacks to be a class instead of mixin
b9dafc9 
This paves the way to more carefully restrict running of callbacks
(preventing them after the editor is destroyed, e.g.).

Refs #319
bantic added a commit that referenced this issue Apr 5, 2016
@bantic
[CLEANUP] Refactor lifecycle callbacks to be a class instead of mixin
1506741 
This paves the way to more carefully restrict running of callbacks
(preventing them after the editor is destroyed, e.g.).

Refs #319
bantic added a commit that referenced this issue Apr 6, 2016
@bantic
[CLEANUP] Refactor lifecycle callbacks to be a class instead of mixin
b337b53 
This paves the way to more carefully restrict running of callbacks
(preventing them after the editor is destroyed, e.g.).

Refs #319
bantic added a commit that referenced this issue Apr 6, 2016
@bantic
Test that cursorDidChange callback not fired after editor is destroyed
7dc7f8d 
refs #319
bantic added a commit that referenced this issue Apr 6, 2016
@bantic
Add Editor#postDidChange, deprecate on('update')
0c1ad10 
refs #319
bantic added a commit that referenced this issue Apr 6, 2016
@bantic
Add Editor#stateDidChange, refactor state change tracking
b10695f 
Also add Editor#_notifyRangeChange (supersedes Editor#_resetRange), used
to notify the editor that the range may have changed and it should
re-check for cursor and state changes.

refs #319
bantic added a commit that referenced this issue Apr 6, 2016
@bantic
Add Editor#stateDidChange, refactor state change tracking
281caf7 
Also add Editor#_notifyRangeChange (supersedes Editor#_resetRange), used
to notify the editor that the range may have changed and it should
re-check for cursor and state changes.

refs #319
@bantic bantic mentioned this issue Apr 6, 2016
Closed
bantic added a commit that referenced this issue Apr 7, 2016
@bantic
Add Editor#stateDidChange, refactor state change tracking
427e238 
Also add Editor#_notifyRangeChange (supersedes Editor#_resetRange), used
to notify the editor that the range may have changed and it should
re-check for cursor and state changes.

refs #319
bantic added a commit that referenced this issue Apr 7, 2016
@bantic
Add Editor#stateDidChange, refactor state change tracking
00a63dd 
Also add Editor#_notifyRangeChange (supersedes Editor#_resetRange), used
to notify the editor that the range may have changed and it should
re-check for cursor and state changes.

refs #319
bantic added a commit that referenced this issue Apr 7, 2016
@bantic
Add Editor#stateDidChange, refactor state change tracking
6164286 
Also add Editor#_notifyRangeChange (supersedes Editor#_resetRange), used
to notify the editor that the range may have changed and it should
re-check for cursor and state changes.

refs #319
bantic added a commit that referenced this issue Apr 7, 2016
@bantic
Test that postDidChange called when card is removed or payload changes
3b159f9 
refs #319
bantic added a commit that referenced this issue Apr 7, 2016
@bantic
[FEATURE] [BUGFIX] Refactor editor hooks
8d6bc6c 
  * 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 `stateDidChange` hook -- called when editor state
    (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 state (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 state
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 state 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* state change
that doesn't change the cursor will fire `cursorDidChange`).

This PR introduces a new `stateDidChange` hook that only fires on state
changes. This cleanly separates the ability to listen for cursor changes
from listening for state changes. It is also more efficient for a consumer to use as a
hook (because most cursor changes do not change state at all, so
listening to that hook as a proxy for detecting state 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 mentioned this issue Apr 7, 2016
Merged
bantic added a commit that referenced this issue Apr 12, 2016
@bantic
[FEATURE] [BUGFIX] Refactor editor hooks
de52092 
  * 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bantic bantic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[FEATURE] [BUGFIX] Refactor editor hooks bustle/mobiledoc-kit
1 participant
@bantic