Make annotation visibility level more explicit #2322

Merged
merged 1 commit into from Jun 19, 2015

Projects

None yet

3 participants

@JakeHartnell
Contributor
  • Meant to resolve #1691 and #2285
  • Privacy directives calls setLevel on the parent annotation's scope to display a clarifying message about who will be able to see the annotation
    on save.
  • Small styling refinements have been added to make the annotation-license and the newly added clarifying text the same size.
  • The word "Private" has been added to the lock icon.

Private annotations are called out with the lock-icon and text:
Private annotation

A clarifying note is added when editing/creating annotation, explicitly stating what the annotation visibility level is:
Making a new annotation

@JakeHartnell JakeHartnell added the WIP label Jun 15, 2015
@tilgovi tilgovi and 1 other commented on an outdated diff Jun 15, 2015
h/templates/client/annotation.html
@@ -97,6 +97,13 @@
</div>
<!-- / Tags -->
+<div class="annotation-section small" ng-if="vm.editing">
+ <p ng-show="level.text == 'Only Me'">
@tilgovi
tilgovi Jun 15, 2015 Contributor

You can just use vm.isPrivate() here and then you don't need setLevel and you don't need to reach "up" out of the isolated privacy directive scope (which is naughty thing to do).

@JakeHartnell
JakeHartnell Jun 15, 2015 Contributor

vm.isPrivate( ) works, but doesn't change when you change the privacy control, only on save. The message should change when you change the visibility level in the privacy directive.

gif of this behavior

@JakeHartnell
JakeHartnell Jun 15, 2015 Contributor

In other words, this scope of the help text should be tied to the current level of the Privacy directive. The way things are currently written, does it make sense that current state of the privacy directive is so separate from the annotation directive?

Should I try binding a property from the annotation controller to the isolated scope of the privacy directive?

Example:

scope:
    level: '='

Or is this also a form of 'reaching up' that I should avoid?

@landscape-bot

Code Health
Code quality remained the same when pulling 9ed0025 on hypothesis:explicitly-state-visibility-level into 501f1e8 on hypothesis:master.

@JakeHartnell JakeHartnell changed the title from Explicitly state annotation visibility level when editing to Make annotation visibility level more explicit Jun 15, 2015
@landscape-bot

Code Health
Code quality remained the same when pulling 15f6580 on hypothesis:explicitly-state-visibility-level into 501f1e8 on hypothesis:master.

@tilgovi
Contributor
tilgovi commented Jun 15, 2015

You're absolutely right, of course, and I'm sorry to have lead you down that dead end.

Using the two-way binding in the isolated scope is fine, but then you have to pass the value through an attribute on the privacy directive, e.g. <privacy level="vm.privacyLevel">. That's a fine way to approach it, although I start to wonder whether the privacy directive is actually still usefully seen as an isolated component, but I suppose you needn't refactor that all now.

@JakeHartnell JakeHartnell removed the WIP label Jun 18, 2015
@JakeHartnell
Contributor

Ok, it's now using two way binding.

@tilgovi
Contributor
tilgovi commented Jun 18, 2015

I think some might disagree, but since the value isn't actually used by the controller at all but is only ever used locally in the annotation template maybe just remove the line from the controller and skip the vm. namespace. I'm fine with template-local things. We don't get the testability that having an API on the controller would provide, but such is the nature of markup. One day UI tests or end-to-end tests could cover this.

@landscape-bot

Code Health
Repository health decreased by 0.01% when pulling 9ce4696 on explicitly-state-visibility-level into d17ed5a on master.

@landscape-bot

Code Health
Repository health decreased by 0.01% when pulling 34a315a on explicitly-state-visibility-level into d17ed5a on master.

@JakeHartnell
Contributor

I think some might disagree, but since the value isn't actually used by the controller at all but is only ever used locally in the annotation template maybe just remove the line from the controller and skip the vm. namespace.

I tried this, but couldn't get it to work.

@JakeHartnell @tilgovi JakeHartnell Make annotation permission levels more clear
- Private annotations have the word private as well as the lock icon on the card
- Small styling refinements have been added to make the annotation-license
- Show the word private alongside lock icon.
- Show message explain visibility setting when editing.
9388b77
@tilgovi
Contributor
tilgovi commented Jun 19, 2015

It was a bad suggestion on my part. All template variables should be namespaced because otherwise they get shadowed all over the place, even by things like ng-if.

I rebased it; moved the initializer to null to a controller property (you had it as $scope.privacylevel whereas the controller in the template is vm, or this in the controller); and camelCased privacyLevel.

@tilgovi tilgovi merged commit c50b946 into master Jun 19, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@tilgovi tilgovi deleted the explicitly-state-visibility-level branch Jun 19, 2015
@landscape-bot

Code Health
Code quality remained the same when pulling 9388b77 on hypothesis:explicitly-state-visibility-level into c9aa105 on hypothesis:master.

@JakeHartnell
Contributor

Thanks @tilgovi!

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