/h

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 annotation controller, part 1 #2761

Merged
merged 8 commits into from Dec 3, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@seanh @robertknight
@seanh
Contributor

seanh commented Dec 2, 2015

Here I'm just fixing various low-hanging fruits / tidying up before attempting more serious refactoring

@seanh seanh added the WIP label Dec 2, 2015

@seanh seanh self-assigned this Dec 2, 2015

Show outdated Hide outdated h/static/scripts/directive/annotation.js

seanh added some commits Dec 1, 2015

@seanh
Just pass isSidebar into AnnotationController normally 
It does not change after directive linking, there's no reason to
$observe() it.
8f5a5f8
@seanh
Move share() into AnnotationController 
Move scope.share() from the annotation directive to vm.share() in
the AnnotationController. Just trying to move as much as possible out of
the directive's scope and link function into the controller, if we're
gonna use a directive controller here try to just use it instead of
randomly using the link function and scope for some things and the
controller for others.
c253720
@seanh
Replace editing with editing() 
Replace AnnotationController's vm.editing boolean with a vm.editing()
method.

This removes some duplicated state, as both vm.editing and vm.action
were recording whether or not the annotation was being edited.
48b526a
@seanh
Remove vm.preview 
Remove AnnotationController's vm.preview as it doesn't appear to be
used.
a35bb6b
@seanh
Move "Save on Ctrl+Enter" into AnnotationController 
Move the DOM event listener function that saves an annotation on
Ctrl+Enter into AnnotationController. This is just for the sake of
keeping all the code in one place (in AnnotationController, not in the
directive's link function) and putting code where it can be tested more
easily.

Note that the controller's save() function was previously called with
scope.$evalAsync() and we're now just calling it synchronously. This
seems to be fine.
bdd84c9
@seanh
Add a FIXME comment
5ea208a
@seanh
Remove an unnecessary check 
It doesn't appear to be possible for model to be null.
1a547b8
@seanh
Move feature() into AnnotationController 
Move scope.feature() from the annotation directive to vm.feature() in
the AnnotationController. Just trying to move as much as possible out of
the directive's scope and link function into the controller, if we're
gonna use a directive controller here try to just use it instead of
randomly using the link function and scope for some things and the
controller for others.
Loading status checks…
712c9aa

@seanh seanh removed the WIP label Dec 3, 2015

@seanh seanh removed their assignment Dec 3, 2015

robertknight added a commit that referenced this pull request Dec 3, 2015

@robertknight
Merge pull request #2761 from hypothesis/2728-refactor-annotation-con… 
…troller

Refactor annotation controller, part 1
100bfcc

@robertknight robertknight merged commit 100bfcc into master Dec 3, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@robertknight robertknight deleted the 2728-refactor-annotation-controller branch Dec 3, 2015

@seanh seanh referenced this pull request Dec 9, 2015

Merged

Annotation controller domain model view model refactor #2781

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