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

Refactor and add tests for annotation editor toolbar commands #3017

Merged
merged 2 commits into from
Feb 29, 2016

Conversation

robertknight
Copy link
Member

This is part of a cleanup of the markdown editor to make fixing the related UX bugs for this sprint easier.

This commit refactors the markdown editor's toolbar command handling by separating out the logic for applying commands (make selection bold, insert link, convert selected lines to a list etc.) and the logic for actually applying that to the input field in the markdown editor.

I also removed a couple of $timeout calls which trigger an unnecessary full $digest cycle. As a temporary measure this will make the UI more responsive when typing in the editor on pages with large numbers of annotations.

@robertknight robertknight force-pushed the editor-markdown-commands-refactor branch 2 times, most recently from 8b26a11 to 5d6604c Compare February 25, 2016 11:08
@nickstenning
Copy link
Contributor

I'm not sure if this is a bug introduced by this code, but the "quote"/blockquote button doesn't update the selection appropriately to account for the new characters at the start of the line.

@nickstenning
Copy link
Contributor

The same is true of the ul/ol buttons. I guess none of these are really intended to be used with a selection, but it seems like it might be nice to preserve it, regardless.

 * Separate the logic for transforming the input field
   from the logic for actually reading the state of
   the input field and applying changes.

   This enables testing of the commands without
   instantiating the component.

   Toolbar commands are functions which take
   the current state of the input field and return
   the updated state.

 * Add tests for the individual commands and use
   of the commands in the markdown editor.
Previously all modified lines in a block were replaced in one
call to replaceText() when applying block formatting, which
lost the selection.

This rewrites the command to transform each line separately
which preserves the selection.
@robertknight robertknight force-pushed the editor-markdown-commands-refactor branch from 5d6604c to 67409c8 Compare February 26, 2016 17:01
@robertknight
Copy link
Member Author

The same is true of the ul/ol buttons. I guess none of these are really intended
to be used with a selection, but it seems like it might be nice to preserve it, regardless.

Agreed. I've rewritten the block formatting command to preserve the selection.

@codecov-io
Copy link

Current coverage is 68.10%

Merging #3017 into master will not affect coverage as of 36319a1

@@            master   #3017   diff @@
======================================
  Files          111     111       
  Stmts         4132    4132       
  Branches       461     461       
  Methods          0       0       
======================================
  Hit           2814    2814       
  Partial         73      73       
  Missed        1245    1245       

Review entire Coverage Diff as of 36319a1

Powered by Codecov. Updated on successful CI builds.

@nickstenning
Copy link
Contributor

Great! FWIW, I really like the parseState/formatState helpers in the tests. Making it easy to write good tests makes me happy.

@nickstenning
Copy link
Contributor

LGTM.

nickstenning added a commit that referenced this pull request Feb 29, 2016
…actor

Refactor and add tests for annotation editor toolbar commands
@nickstenning nickstenning merged commit 9dae998 into master Feb 29, 2016
@nickstenning nickstenning deleted the editor-markdown-commands-refactor branch February 29, 2016 11:01
@robertknight
Copy link
Member Author

Making it easy to write good tests makes me happy.

This is one of the reasons I find functional approaches to front-end architecture so interesting. The logic which can be expressed in a functional way becomes very easy to test.

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.

None yet

3 participants