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

Show "read more" link on overly long in-stream statuses #8205

Merged
merged 6 commits into from
Sep 27, 2018
Merged

Show "read more" link on overly long in-stream statuses #8205

merged 6 commits into from
Sep 27, 2018

Conversation

lanodan
Copy link
Contributor

@lanodan lanodan commented Aug 14, 2018

Collapsed:

image

Extended:

image

Note: originally done for mastofe of pleroma, on this MR: https://git.pleroma.social/pleroma/mastofe/merge_requests/12

And I won’t fix issues on this PR, this is basically sending the patch to upstream.

@lanodan lanodan changed the title Backport https://github.com/tootsuite/mastodon/pull/658 Forward-port https://github.com/tootsuite/mastodon/pull/658 Aug 14, 2018
@lanodan
Copy link
Contributor Author

lanodan commented Aug 15, 2018

Here it is with the spoiler button colors.
collapsed:
screen

expanded:
screen

Note: The general base UI colors are slightly different because of the pleroma theme.

@Tagadda
Copy link

Tagadda commented Aug 15, 2018

Some screenshot of the Glitch Social's Collapsible toots feature.

Text only

image

image

Text with image.

image

image

One of the most useful feature : auto-collapse boost/fav notifications.

image

The settings

image

EDIT: adding spoiliers

@lanodan
Copy link
Contributor Author

lanodan commented Aug 15, 2018

@Tagadda : I’m not sure why you’re posting screens of glitch-soc here, it’s likely not based on this or highly derived (the original Pull-Request being more than one year old).

@kaniini
Copy link
Contributor

kaniini commented Aug 15, 2018

i will literally pay @Gargron to merge this

@noiob
Copy link
Contributor

noiob commented Aug 15, 2018

This is cool, but I much prefer the way glitch-soc handles this, though I would prefer it if glitch-soc could show a few more lines in the timeline

@lanodan
Copy link
Contributor Author

lanodan commented Aug 15, 2018

The number of lines shown when collapsed or when a status should collide is quite configurable in the PR’s logic and CSS.
The other glitch-soc stuff like what should be collapsed and stuff like that is probably not easily configurable due to the way that Tootsuite’s Mastodon does the all settings outside of the UI, where glitch-soc presents some in the UI.

@rugk
Copy link

rugk commented Aug 15, 2018

Maybe make the "expand button" less visually invasive. It could have a transparent background color, or some "fading" (transparent gradient) effect or so.

@kaniini
Copy link
Contributor

kaniini commented Aug 15, 2018

to be clear, the cash bounty offered is covering either this patch or a similar patch which duplicates the functionality from glitch-soc. it doesn't matter to me which one happens.

@lanodan
Copy link
Contributor Author

lanodan commented Aug 16, 2018

And I won’t fix issues on this PR, this is basically sending the patch to upstream.

I’m rephrasing this one to: I will not introduce/write some specific tootsuite/mastodon changes to the code, one exception was the codeclimate issue because it was a simple one and would allow to more easily take it. But, I will welcome patches to this PR/branch that can be used on pleroma’s too, for example rugk’s idea.


Sidenote: I will be away from the internet until Monday(the 20th).

@ghost
Copy link

ghost commented Aug 20, 2018

I’m definitely going to steal this patch for banana.dog :).

@ghost
Copy link

ghost commented Aug 20, 2018

@lanodan I applied this to master and if I make a long toot with a content warning I can't see the full text once I expand the content warning. In this screenshot the two toots should be identical but the first one is chopped off
screenshot 2018-08-20 16 07 02

I fixed this issue for me with a small change to status_content.js
https://github.com/usbsnowcrash/mastodon/commit/eb2ba54a24efab46caf07c4d0a74f8053ec1706e

@lanodan
Copy link
Contributor Author

lanodan commented Aug 20, 2018

I have other few bugs on this one…
Namely:

  • In thread view non-focused posts are shorted (actually looks more hard to reproduce than this…)
  • On a specific height it triggers but opening/closing doesn’t seem to change the height (should add some margin to the logic?)

@lanodan
Copy link
Contributor Author

lanodan commented Aug 21, 2018

I fixed the issue with long posts already having a Content-Warning.

@ghost
Copy link

ghost commented Aug 22, 2018

I am running these changes on https://banana.dog right now. There are def still some weird issues (like the expansion thing appearing but clicking it having no effect because the full text of the message is already on display)
screenshot 2018-08-22 17 31 54
screenshot 2018-08-22 17 31 41
screenshot 2018-08-22 17 31 35

@lanodan
Copy link
Contributor Author

lanodan commented Aug 22, 2018

This no-effect collapsing issue is quite a weird one, I think it’s because it’s checking on the wrong element’s height or something like that.

@ghost
Copy link

ghost commented Aug 25, 2018

@lanodan I also added this as an optional feature that the admin can turn on or off in my branch. I have not seen the bug I mentioned above in some time (as of my latest build).

I quite like the feature right now and I hope @Gargron considers it, especially as an optional feature. It is really nice.

@Gargron
Copy link
Member

Gargron commented Aug 26, 2018

Please consider this patch:

image

From c55b121cb0a44a500a2c868681e9b067f6541e96 Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Sun, 26 Aug 2018 22:42:08 +0200
Subject: [PATCH] Improve styling, wording, and behaviour

Remove collapse/expand behaviour, "read more" button leads to open
the full status in detailed view. No need to collapse in detailed
view.

Use real line-height to calculate cut-off
---
 .../mastodon/components/status_content.js          | 42 ++++++++++--------
 app/javascript/styles/mastodon/components.scss     | 51 +++++-----------------
 2 files changed, 35 insertions(+), 58 deletions(-)

diff --git a/app/javascript/mastodon/components/status_content.js b/app/javascript/mastodon/components/status_content.js
index 6aa973d..f221474 100644
--- a/app/javascript/mastodon/components/status_content.js
+++ b/app/javascript/mastodon/components/status_content.js
@@ -6,6 +6,8 @@ import { FormattedMessage } from 'react-intl';
 import Permalink from './permalink';
 import classnames from 'classnames';
 
+const MAX_HEIGHT = 322; // 20px * 16 (+ 2px padding at the top)
+
 export default class StatusContent extends React.PureComponent {
 
   static contextTypes = {
@@ -22,9 +24,8 @@ export default class StatusContent extends React.PureComponent {
 
   state = {
     hidden: true,
-    collapsed: null,
+    collapsed: null, //  `collapsed: null` indicates that an element doesn't need collapsing, while `true` or `false` indicates that it does (and is/isn't).
   };
-  //  `collapsed: null` indicates that an element doesn't need collapsing, while `true` or `false` indicates that it does (and is/isn't).
 
   _updateStatusLinks () {
     const node = this.node;
@@ -59,10 +60,13 @@ export default class StatusContent extends React.PureComponent {
 
     if (
       this.props.collapsable
+      && this.props.onClick
       && this.state.collapsed === null
-      && node.clientHeight > 200
+      && node.clientHeight > MAX_HEIGHT
       && this.props.status.get('spoiler_text').length === 0
-    ) this.setState({ collapsed: true });
+    ) {
+      this.setState({ collapsed: true });
+    }
   }
 
   componentDidMount () {
@@ -148,13 +152,18 @@ export default class StatusContent extends React.PureComponent {
       'status__content--with-action': this.props.onClick && this.context.router,
       'status__content--with-spoiler': status.get('spoiler_text').length > 0,
       'status__content--collapsed': this.state.collapsed === true,
-      'status__content--expanded': this.state.collapsed === false,
     });
 
     if (isRtl(status.get('search_index'))) {
       directionStyle.direction = 'rtl';
     }
 
+    const readMoreButton = (
+      <button className='status__content__read-more-button' onClick={this.props.onClick}>
+        <FormattedMessage id='status.read_more' defaultMessage='Read more' /><i className='fa fa-fw fa-angle-right' />
+      </button>
+    );
+
     if (status.get('spoiler_text').length > 0) {
       let mentionsPlaceholder = '';
 
@@ -184,26 +193,23 @@ export default class StatusContent extends React.PureComponent {
         </div>
       );
     } else if (this.props.onClick) {
-      return (
+      const output = [
         <div
           ref={this.setRef}
           tabIndex='0'
           className={classNames}
           style={directionStyle}
+          dangerouslySetInnerHTML={content}
           onMouseDown={this.handleMouseDown}
           onMouseUp={this.handleMouseUp}
-        >
-          <div dangerouslySetInnerHTML={content} />
-          {this.state.collapsed !== null ?
-            <button
-              className='status__content__collapse-button'
-              onClick={this.handleCollapsedClick}
-            >
-              <i className='fa fa-fw fa-angle-double-down' />
-            </button>
-            : null}
-        </div>
-      );
+        />,
+      ];
+
+      if (this.state.collapsed) {
+        output.push(readMoreButton);
+      }
+
+      return output;
     } else {
       return (
         <div
diff --git a/app/javascript/styles/mastodon/components.scss b/app/javascript/styles/mastodon/components.scss
index 101ff49..037d451 100644
--- a/app/javascript/styles/mastodon/components.scss
+++ b/app/javascript/styles/mastodon/components.scss
@@ -724,51 +724,22 @@
 }
 
 .status__content.status__content--collapsed {
-  padding-bottom: 25px;
-  max-height: 200px;
-
-  i {
-    transform: rotateX(0);
-  }
-}
-
-.status__content.status__content--expanded, {
-  padding-bottom: 25px;
-  height: auto;
-
-  i {
-    transform: rotateX(180deg);
-  }
+  max-height: 20px * 15; // 15 lines is roughly above 500 characters
 }
 
-.status__content__collapse-button {
+.status__content__read-more-button {
   display: block;
-  position: absolute;
-  bottom: 0;
-  left: 0;
-  right: 0;
-  width: 100%;
-  height: 25px;
-  font-size: 18px;
-  line-height: 25px;
-  color: $inverted-text-color;
-  text-align: center;
-  background: $action-button-color;
-  transition: background .2s ease-in-out, color .2s ease-in-out;
+  font-size: 15px;
+  line-height: 20px;
+  color: lighten($ui-highlight-color, 8%);
   border: 0;
-  border-radius: 2px;
-
-  &:hover {
-    background: lighten($action-button-color, 7%);
-  }
-
-  i {
-    transition: transform .2s ease-in-out;
+  background: transparent;
+  padding: 0;
+  padding-top: 8px;
 
-    &,
-    &:hover {
-      color: $inverted-text-color !important;
-    }
+  &:hover,
+  &:active {
+    text-decoration: underline;
   }
 }
 
-- 
2.7.4
  • I don't think there is a need to toggle the collapsed state when it can simply be opened in detailed view, therefore "read more"
  • The max-height is based on the actual line-height of the text inside so we don't get lines that are cut off in the middle
  • The cut off only kicks in for text that is roughly a line longer than 500 characters, so should not appear in most cases

@rugk
Copy link

rugk commented Aug 26, 2018

@Gargron So what about including this patch (as an option) in Mastodon? 😃

@renatolond
Copy link
Contributor

LGTM, @Gargron

@lanodan
Copy link
Contributor Author

lanodan commented Aug 27, 2018

I accepted your patch as it’s your software anyway. But here is my opinion about it.

  • Having a actual button instead of a link makes it looks like something that belongs in the UI rather than something the post could have made.
  • A “show more” allows to:
    • avoid “overriding” the detailed view
    • avoid moving from one end of the UI to another, making it a large motion and potentially breaking timeline context
    • see the entire thread expanded (or not) in the detailed view (maybe you should fix the logic there?)
      screen

Anyway, thanks for fixing the line-height part even if it makes it quite longer.

lanodan and others added 6 commits August 27, 2018 14:25
@lanodan
Copy link
Contributor Author

lanodan commented Sep 26, 2018

@Gargron Any update on merging this Pull Request? It’s been about a month since the last update on your side.

@Gargron Gargron changed the title Forward-port https://github.com/tootsuite/mastodon/pull/658 Show "read more" link on overly long in-stream statuses Sep 27, 2018
@Gargron Gargron merged commit 15fc2b7 into mastodon:master Sep 27, 2018
@rugk
Copy link

rugk commented Sep 27, 2018

🎉 👍

panarom added a commit to panarom/mastodon that referenced this pull request Dec 28, 2019
…set of status updates

in order to limit the maximum size of a status in a list view (e.g. the home timeline), so as to avoid having to scroll all the way through an abnormally large status update (see mastodon#8205), the following steps are taken:
•the element containing the status is rendered in the browser
•its height is calculated, to determine if it exceeds the maximum height threshold.
Unfortunately for performance, these steps are carried out in the componentDidMount(/Update) method, which also performs style modifications on the element.  The combination of  height request and style modification during javascript evaluation in the browser leads to layout-thrashing, where the elements are repeatedly re-laid-out (see https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing & https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers).
The solution implemented here is to memoize the collapsed state in Redux the first time the status is seen (e.g. when fetched as part of a small batch, to populate the home timeline) , so that on subsequent re-renders, the value can be queried, rather than recalculated.  This strategy is derived from mastodon#4439 & mastodon#4909, and should resolve mastodon#12455.

Andrew Lin (https://github.com/onethreeseven) is thanked for his assistance in root cause analysis and solution brainstorming
Gargron pushed a commit that referenced this pull request Dec 29, 2019
#12661)

* Summary: fix slowness due to layout thrashing when reloading a large set of status updates

in order to limit the maximum size of a status in a list view (e.g. the home timeline), so as to avoid having to scroll all the way through an abnormally large status update (see #8205), the following steps are taken:
•the element containing the status is rendered in the browser
•its height is calculated, to determine if it exceeds the maximum height threshold.
Unfortunately for performance, these steps are carried out in the componentDidMount(/Update) method, which also performs style modifications on the element.  The combination of  height request and style modification during javascript evaluation in the browser leads to layout-thrashing, where the elements are repeatedly re-laid-out (see https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing & https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers).
The solution implemented here is to memoize the collapsed state in Redux the first time the status is seen (e.g. when fetched as part of a small batch, to populate the home timeline) , so that on subsequent re-renders, the value can be queried, rather than recalculated.  This strategy is derived from #4439 & #4909, and should resolve #12455.

Andrew Lin (https://github.com/onethreeseven) is thanked for his assistance in root cause analysis and solution brainstorming

* remove getSnapshotBeforeUpdate from status

* remove componentWillUnmount from status

* persist last-intersected status update and restore when ScrollableList is restored

e.g. when navigating from home-timeline to a status conversational  thread and <Back again

* cache currently-viewing status id to avoid calling redux with identical value

* refactor collapse toggle to pass explicit boolean
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.

7 participants