Visually truncate long bodies and quotes #2451

JakeHartnell · 2015-08-18T10:49:45Z

Introduce collapsing bodies and quotes which expand when you click more.
This is done through the new excerpt directive which looks to see if text is overflowing its container and appends 'More' and 'Less' spans.
Watch annotation controller to see if an annotation is being edited so that excerpts can be applied to newly created and updated annotation.
Add event for threadToggleCollapse in the thread directive, and evaluate excerpts after thread expansion.
Implements @conordelahunty's design: https://hypothes-is.slack.com/files/conor/F09H4G4LC/pasted_image_at_2015_08_24_06_38_pm.png

It looks like this:

BigBlueHat · 2015-08-20T15:53:14Z

Needs angular wonk code review. 😉 I'm 👍 on this as it solves the > 2 year old #487 issue.

@conordelahunty could you take a look at this also? Cheers!

tilgovi · 2015-08-20T16:29:48Z

h/static/styles/excerpt.scss

+      content: " ";
+      margin-left: -3em;
+      padding-right: 40px;
+      background: -webkit-gradient(linear, left top, right top, from(rgba(255, 255, 255, 0)), to(white), color-stop(50%, white));


http://compass-style.org/reference/compass/css3/images/

Good shout!

landscape-bot · 2015-08-26T16:11:49Z

Code quality remained the same when pulling 16bc4b7 on hypothesis:visual-truncation into 59aeb92 on hypothesis:master.

JakeHartnell · 2015-09-08T20:27:34Z

Ok, after much ado, I'm hoping this is good enough to be merged soon. 🙏

nickstenning · 2015-09-15T16:28:32Z

h/static/scripts/app.coffee

@@ -92,6 +92,7 @@ module.exports = angular.module('h', [

 .directive('annotation', require('./directive/annotation'))
 .directive('deepCount', require('./directive/deep-count'))
+.directive('excerpt', require('./directive/excerpt'))


Big 👍 to trying to keep this functionality as a separate directive.

nickstenning · 2015-09-15T16:43:44Z

The styling/markup parts of this look absolutely fine to me. I'm a bit concerned about the business logic to do with vm.editing and thread collapse events making its way into the excerpt directive, so I'm going to try and put together a quick demo of how it might be possible to avoid that.

nickstenning · 2015-09-15T18:45:39Z

Here's an example of what I mean. It's a bit rough around the edges but it shows how you might be able to keep the disable/enable state as a parameter to the excerpt directive rather than including logic about the rest of the application.

Hope that helps!

tilgovi · 2015-09-15T19:38:08Z

👍 @nickstenning, and I would only tweak it to prefer positive rather than negative conditionals as I think they're easier to parse: e.g. excerpt-if rather than excerpt-disable-if and default it to true so that it's an optional attribute.

nickstenning · 2015-09-16T11:35:05Z

tweak it to prefer positive rather than negative conditionals

👍

nickstenning · 2015-09-17T09:37:32Z

@JakeHartnell If you want to walk through the plunker I linked above at some point just let me know. I'm happy to help in any way I can to get this finished.

landscape-bot · 2015-09-19T11:30:11Z

Repository health increased by 0.22% when pulling cb032fb on visual-truncation into 4a324db on master.

42 new problems were found (including 0 errors and 5 code smells).
68 problems were fixed (including 0 errors and 8 code smells).

JakeHartnell · 2015-09-21T21:44:13Z

@nickstenning this is great, and I understand why you like this approach. I'm having one problem that I had early on when I tried a similar approach that also used transclusion. The markdown converter also transcludes the annotation-body section and throws the following error:

"Error: [$compile:multidir] Multiple directives [excerpt, markdown] asking for template on: <section name="text" class="annotation-body" excerpt="" excerpt-if="!vm.editing" ng-model="vm.annotation.text" ng-readonly="!vm.editing" markdown="">

Any thoughts on that? I'll keep working on it, but I think if we can get past that we have a nice modular solution here. Thanks for the awesome comments and the plunker.

tilgovi · 2015-09-21T21:57:18Z

Unfortunately, the way around that is just another wrapping element so that
these directives don't conflict by being used on the same element.

On Mon, Sep 21, 2015, 14:44 Jake Hartnell [email protected] wrote:

@nickstenning https://github.com/nickstenning this is great, and I get
why you like this approach. I'm having one problem that I had early on when
I tried a similar approach that also used transclusion. The markdown
converter also transcludes the annotation-body section and throws the
following error:

"Error: [$compile:multidir] Multiple directives [excerpt, markdown] asking for template on:

Any thoughts on that? I'll keep working on it, but I think if we can get
past that we have a nice modular solution here. Thanks for the awesome
comments and the plunker.

—
Reply to this email directly or view it on GitHub
#2451 (comment).

JakeHartnell · 2015-09-21T22:51:41Z

Unfortunately, the way around that is just another wrapping element so that these directives don't conflict by being used on the same element.

Cool, thanks.

nickstenning · 2015-09-22T09:22:05Z

h/static/scripts/directive/excerpt.coffee

+
+  scope:
+    enabled: '=excerptIf'
+    maxheight: '=maxheight'


This doesn't need to go here, as far as I can tell. You can add some rules like .annotation-body .excerpt { max-height: 16.2em } to the CSS.

Funny enough, that's what I was going to do, but worried it wouldn't be modular enough.

nickstenning · 2015-09-22T09:26:36Z

Ok, looking a lot better. But checking it out there seem to be a few bugs -- excerpt controls seem to be displayed all the time for quotes, regardless of how long they are.

I've added a couple of comments inline, and I also would like to see the template moved into h/templates/client/ and added to h/templates/app.html along with all our other client templates.

nickstenning · 2015-10-08T15:48:23Z

Okay, I've just force-pushed this branch. I've simplified the excerpt directive, translated it to JavaScript (although "translated" is probably a strong word in this case), and cleaned up the CSS.

@robertknight perhaps you could take a look at the general approach, and if you're happy I'll sort out the tests.

nickstenning · 2015-10-08T15:48:45Z

Oh, and the whole thing is now behind a feature flag.

ghost · 2015-10-08T16:07:13Z

Cool, I don't know if Jake got around to make the design changes or not yet but maybe Robert can pick this up if not.

robertknight · 2015-10-09T08:26:30Z

h/static/styles/excerpt.scss

@@ -0,0 +1,31 @@
+@import "compass/css3/images";
+@import "variables";


IIRC this will pull in a second copy of 'variables.scss' rather than require them once. I want to use a system in future where requiring in CSS works the same way as JS modules.

I'm guessing that's not actually a problem because variables.scss is just variable definitions, but you're absolutely right -- it's not necessary. I'll remove it.

robertknight · 2015-10-09T08:46:23Z

@nickstenning This looks good to me.
@conordelahunty - What are the design changes you wanted?

landscape-bot · 2015-10-09T10:12:35Z

Code quality remained the same when pulling d15accf on hypothesis:visual-truncation into 81ca666 on hypothesis:master.

nickstenning · 2015-10-09T10:14:02Z

Thanks for the review @robertknight. I'll fix up these things, write some tests, and then I reckon we can probably merge this and fix up the design in a separate PR, now that it's feature-flagged.

Introduces auto-truncation of long bodies and quotes which expand when you click on a "More" link. Feature flagged as 'truncate_annotations'.

nickstenning · 2015-10-09T13:36:04Z

@robertknight Right, that's everything except the resize behaviour fixed. I reckon we can fix that another day. Let me know what you think.

robertknight · 2015-10-09T14:59:20Z

h/static/styles/common.scss

@@ -5,6 +5,7 @@
 @import 'mixins/responsive';
 @import 'grid';
 @import 'annotations';
+@import 'excerpt';


This should go under 'components' in app.scss, assuming it isn't used by the help page.

Visually truncate long bodies and quotes

#2451 (comment)

JakeHartnell · 2015-10-09T19:18:33Z

Thanks @robertknight and @nickstenning!

tilgovi · 2015-10-30T00:20:23Z

This is also not enabled in production, it seems.

dwhly · 2015-10-30T00:58:18Z

@conordelahunty is still working w/ the team to get his tweaks to the style implemented.

JakeHartnell · 2015-10-30T01:03:59Z

@conordelahunty sorry I didn't get around to those, but it was good to see it on stage being used. Definitely thing we need to make the annotation bodies a bit bigger, as images are cut off.

judell · 2015-10-30T23:54:44Z

It's also true that for a textual annotation, More may only reveal a few
characters.

On Thu, Oct 29, 2015 at 6:03 PM, Jake Hartnell [email protected]
wrote:

@conordelahunty https://github.com/conordelahunty sorry I didn't get
around to those, but it was good to see it on stage being used. Definitely
thing we need to make the annotation bodies a bit bigger, as images are cut
off.
[image: annotationbodiesneedabiggermaxheight]
https://cloud.githubusercontent.com/assets/521978/10836135/523488d8-7e67-11e5-9893-37e7687f1d17.jpg

—
Reply to this email directly or view it on GitHub
#2451 (comment).

JakeHartnell force-pushed the visual-truncation branch from afda6cc to 79b6a89 Compare August 18, 2015 10:56

tilgovi reviewed Aug 20, 2015
View reviewed changes

JakeHartnell force-pushed the visual-truncation branch from 79b6a89 to 16bc4b7 Compare August 21, 2015 09:46

JakeHartnell added the WIP label Aug 21, 2015

JakeHartnell force-pushed the visual-truncation branch 2 times, most recently from 929ce15 to 10152be Compare September 8, 2015 19:56

JakeHartnell removed the WIP label Sep 8, 2015

JakeHartnell force-pushed the visual-truncation branch from 10152be to cb032fb Compare September 8, 2015 20:01

nickstenning reviewed Sep 15, 2015
View reviewed changes

JakeHartnell force-pushed the visual-truncation branch from cb032fb to 7fda4f4 Compare September 22, 2015 03:33

nickstenning reviewed Sep 22, 2015
View reviewed changes

JakeHartnell force-pushed the visual-truncation branch from c19dc19 to 7cf68af Compare September 29, 2015 20:34

nickstenning force-pushed the visual-truncation branch from 7cf68af to f4414f7 Compare October 8, 2015 15:43

nickstenning force-pushed the visual-truncation branch from f4414f7 to d15accf Compare October 8, 2015 15:49

robertknight reviewed Oct 9, 2015
View reviewed changes

nickstenning force-pushed the visual-truncation branch from d15accf to bf9fbe3 Compare October 9, 2015 13:15

Truncate long bodies and quotes

4c8f9bf

Introduces auto-truncation of long bodies and quotes which expand when you click on a "More" link. Feature flagged as 'truncate_annotations'.

nickstenning force-pushed the visual-truncation branch from bf9fbe3 to 4c8f9bf Compare October 9, 2015 13:30

robertknight reviewed Oct 9, 2015
View reviewed changes

robertknight added a commit that referenced this pull request Oct 9, 2015

Merge pull request #2451 from hypothesis/visual-truncation

2fda6b8

Visually truncate long bodies and quotes

robertknight merged commit 2fda6b8 into master Oct 9, 2015

nickstenning added a commit that referenced this pull request Oct 9, 2015

Move import of excerpt CSS into app.scss

7e96c1a

#2451 (comment)

nickstenning deleted the visual-truncation branch October 16, 2015 13:45

nickstenning mentioned this pull request Feb 10, 2016

Cards aren't visually truncated #487

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visually truncate long bodies and quotes #2451

Visually truncate long bodies and quotes #2451

JakeHartnell commented Aug 18, 2015

BigBlueHat commented Aug 20, 2015

tilgovi Aug 20, 2015

JakeHartnell Aug 21, 2015

landscape-bot commented Aug 26, 2015

JakeHartnell commented Sep 8, 2015

nickstenning Sep 15, 2015

nickstenning commented Sep 15, 2015

nickstenning commented Sep 15, 2015

tilgovi commented Sep 15, 2015

nickstenning commented Sep 16, 2015

nickstenning commented Sep 17, 2015

landscape-bot commented Sep 19, 2015

JakeHartnell commented Sep 21, 2015

tilgovi commented Sep 21, 2015

JakeHartnell commented Sep 21, 2015

nickstenning Sep 22, 2015

JakeHartnell Sep 22, 2015

nickstenning commented Sep 22, 2015

nickstenning commented Oct 8, 2015

nickstenning commented Oct 8, 2015

ghost commented Oct 8, 2015

robertknight Oct 9, 2015

nickstenning Oct 9, 2015

robertknight commented Oct 9, 2015

landscape-bot commented Oct 9, 2015

nickstenning commented Oct 9, 2015

nickstenning commented Oct 9, 2015

robertknight Oct 9, 2015

JakeHartnell commented Oct 9, 2015

tilgovi commented Oct 30, 2015

dwhly commented Oct 30, 2015

JakeHartnell commented Oct 30, 2015

judell commented Oct 30, 2015

		@@ -0,0 +1,31 @@
		@import "compass/css3/images";
		@import "variables";

Visually truncate long bodies and quotes #2451

Visually truncate long bodies and quotes #2451

Conversation

JakeHartnell commented Aug 18, 2015

BigBlueHat commented Aug 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landscape-bot commented Aug 26, 2015

JakeHartnell commented Sep 8, 2015

Choose a reason for hiding this comment

nickstenning commented Sep 15, 2015

nickstenning commented Sep 15, 2015

tilgovi commented Sep 15, 2015

nickstenning commented Sep 16, 2015

nickstenning commented Sep 17, 2015

landscape-bot commented Sep 19, 2015

JakeHartnell commented Sep 21, 2015

tilgovi commented Sep 21, 2015

JakeHartnell commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickstenning commented Sep 22, 2015

nickstenning commented Oct 8, 2015

nickstenning commented Oct 8, 2015

ghost commented Oct 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Oct 9, 2015

landscape-bot commented Oct 9, 2015

nickstenning commented Oct 9, 2015

nickstenning commented Oct 9, 2015

Choose a reason for hiding this comment

JakeHartnell commented Oct 9, 2015

tilgovi commented Oct 30, 2015

dwhly commented Oct 30, 2015

JakeHartnell commented Oct 30, 2015

judell commented Oct 30, 2015