Extract PostStream state #2160

askvortsov1 · 2020-05-10T06:33:38Z

Fixes Part of #1821, #2144
Fixes #2001
Fixes #2086

Changes proposed in this pull request:
Extract PostStream state out of PostStream, PostStreamScrubber.

Reviewers should focus on:

Can we get rid of the positionChanged event sent out in PostStream?

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).

dsevillamartin

I have some questions about the code and implementation.

Will look more thoroughly once I'm not on mobile.

js/src/forum/components/DiscussionPage.js

js/src/forum/components/RenameDiscussionModal.js

clarkwinkelmann · 2020-06-01T17:07:48Z

I'm mostly ok with this solution, but I'm thinking whether it wouldn't make sense to maybe keep the stream as a single class?

If we simply made the stream into a non-component class, that has just a render() method that outputs vnodes. The only thing that would need moving are the stuff in config(). Those could become a start and stop method that are called from the DiscussionPage component. And the things in init would move to the constructor instead. It would no longer be a component, but just a helper class to generate the output of the DiscussionPage.

It would make for a way simpler diff, and in my opinion this is a valid way to use Mithril (very similar to what I was suggesting with AlertManager or ModalManager).

clarkwinkelmann · 2020-06-28T17:44:19Z

Could you avoid force-pushing during the review phase? It makes it completely impossible to see what was changed since last review 😅 😅 😅

askvortsov1 · 2020-06-28T18:51:37Z

Sorry, that was a rebase to avoid conflicts with other merged things.

w-4 · 2020-07-08T02:55:48Z

js/src/forum/states/PostStreamState.js

+   */
+  reset(start, end) {
+    this.visibleStart = start || 0;
+    this.visibleEnd = this.sanitizeIndex(end || this.constructor.loadCount);


Since doing a major rewrite, might want to also change visibleEnd to:

this.visibleEnd = this.sanitizeIndex(end || this.constructor.loadCount - 1)

Logic:
Suppose loadCount is 1, visibleStart = 0 and visibleEnd should be the same post 0.

Would need to also adjust all the code that references visibleEnd - I can send you a pull request for that if you want.

clarkwinkelmann · 2020-07-28T21:43:48Z

I'm really not sure how to handle review on this. The diff is very complicated to read.

If it works as before I'd say go ahead and we can always fix stuff later.

…ernally (and using it would be bad practice), fixing up PostStream

The event is not triggered anymore. This is now handled through the `positionHandler` prop.

...instead of calculating this derived value outside the state class.

In addition, this again avoids writing a state property from outside the state class. I am not 100% sure whether this extra sanitization is necessary, but it seems to be the only place where it is not applied when changing the value of `visibleEnd` (and not safeguarded otherwise), so I erred on the safe side.

- Law of demeter (no need to access discussion through the state) - Less public API surface of the state object

This is actually relied on already by not passing the parameter in other methods.

It's not clear whether this was intentionally omitted.

This also avoids a double-fade from the JavaScript code, which was probably introduced in #2160.

* Fix closing the composer with ESC key Regression from #2161. * Remove obsolete method Regression from #2162. * Mark method as protected * Fade in posts in post stream using CSS This also avoids a double-fade from the JavaScript code, which was probably introduced in #2160. * Fix fadeIn for post stream items Co-authored-by: Alexander Skvortsov <sasha.skvortsov109@gmail.com>

askvortsov1 requested review from clarkwinkelmann and dsevillamartin May 10, 2020 06:33

askvortsov1 mentioned this pull request May 10, 2020

Frontend Framework Rewrite #1821

Closed

65 tasks

dsevillamartin reviewed May 10, 2020

View reviewed changes

js/src/forum/components/DiscussionPage.js Outdated Show resolved Hide resolved

js/src/forum/components/DiscussionPage.js Outdated Show resolved Hide resolved

js/src/forum/components/RenameDiscussionModal.js Outdated Show resolved Hide resolved

askvortsov1 requested a review from dsevillamartin May 10, 2020 20:56

askvortsov1 linked an issue May 12, 2020 that may be closed by this pull request

Frontend Framework Rewrite #1821

Closed

65 tasks

askvortsov1 removed a link to an issue May 22, 2020

Frontend Framework Rewrite #1821

Closed

65 tasks

franzliedke linked an issue May 22, 2020 that may be closed by this pull request

Frontend Needs Proper State Management #2144

Closed

franzliedke removed a link to an issue May 22, 2020

Frontend Needs Proper State Management #2144

Closed

askvortsov1 requested a review from franzliedke June 19, 2020 00:15

askvortsov1 force-pushed the as/frontend_rewrite_post_stream_state_try_4 branch 2 times, most recently from d6df661 to eb52edc Compare June 19, 2020 21:57

askvortsov1 force-pushed the as/frontend_rewrite_post_stream_state_try_4 branch from eb52edc to 67b6691 Compare June 27, 2020 23:00

franzliedke force-pushed the as/frontend_rewrite_post_stream_state_try_4 branch from 67b6691 to 81fa5bc Compare July 3, 2020 13:12

w-4 reviewed Jul 8, 2020

View reviewed changes

askvortsov1 mentioned this pull request Jul 26, 2020

Link to post scrolls to the previous post #2086

Closed

askvortsov1 and others added 10 commits July 31, 2020 11:17

Extract PostStream state

7af8e35

Remove event from PostState, pass handler in via props,

cff9b32

Moved refresh method of discussionpage into init, as its not used ext…

614bb0d

…ernally (and using it would be bad practice), fixing up PostStream

Revert inlining method, rename the method instead

6bcecd6

Remove obsolete event handler

6c087da

The event is not triggered anymore. This is now handled through the `positionHandler` prop.

Encapsulate viewingEnd() in state

8a9e50d

...instead of calculating this derived value outside the state class.

Pass discussion as prop to stream components

2ec49db

- Law of demeter (no need to access discussion through the state) - Less public API surface of the state object

Large simplifications of PostStreamScrubber

5427b35

Simplify paused logic

9c63c54

askvortsov1 and others added 6 commits July 31, 2020 11:17

Restore old scrubber index calculation system

a850f4a

A bit more cleanup and bugfixes

1d86620

A bit more cleanup

bbd8136

De-propify visible

f99f79e

Don't save index twice in post stream post-load

8c362bf

Revert formatting changes

1a2f952

franzliedke force-pushed the as/frontend_rewrite_post_stream_state_try_4 branch from fb04724 to 1a2f952 Compare July 31, 2020 09:33

franzliedke added 3 commits July 31, 2020 12:06

Fix docblock

aae6f24

Rename props

3ce9475

Remove unused prop

09c722e

franzliedke force-pushed the as/frontend_rewrite_post_stream_state_try_4 branch from c13ff12 to 09c722e Compare July 31, 2020 10:14

franzliedke added 8 commits July 31, 2020 13:21

Fix docblocks

8b69b24

Move method to previous position

57f5ad4

Encapsulate a bit more logic in the state

0138413

Add default parameter value

a044c64

This is actually relied on already by not passing the parameter in other methods.

Restore old implementation of goToLast()

e455e6c

It's not clear whether this was intentionally omitted.

Unify pausing

e8394e4

Start decoupling scrolling from state

6f6a09d

Default parameter value

d4abf58

askvortsov1 force-pushed the as/frontend_rewrite_post_stream_state_try_4 branch from fc5eddb to d4abf58 Compare August 4, 2020 22:07

askvortsov1 merged commit 6953d93 into master Aug 8, 2020

askvortsov1 deleted the as/frontend_rewrite_post_stream_state_try_4 branch August 8, 2020 18:45

franzliedke added a commit that referenced this pull request Aug 14, 2020

Fade in posts in post stream using CSS

90707a4

This also avoids a double-fade from the JavaScript code, which was probably introduced in #2160.

This was referenced Oct 2, 2020

Scrollbar not showing accurate values #1535

Closed

post number changes on page refresh #789

Closed

Jumping to a specific post number is unreliable? #437

Closed

Make PostStreamScrubber work for Posts that have top margin #2369

Merged

askvortsov1 mentioned this pull request Mar 10, 2022

Images in posts can throw off PostStream scroll flarum/issue-archive#395

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract PostStream state #2160

Extract PostStream state #2160

askvortsov1 commented May 10, 2020 •

edited

Loading

dsevillamartin left a comment

clarkwinkelmann commented Jun 1, 2020

clarkwinkelmann commented Jun 28, 2020

askvortsov1 commented Jun 28, 2020

w-4 Jul 8, 2020

clarkwinkelmann commented Jul 28, 2020

Extract PostStream state #2160

Extract PostStream state #2160

Conversation

askvortsov1 commented May 10, 2020 • edited Loading

dsevillamartin left a comment

Choose a reason for hiding this comment

clarkwinkelmann commented Jun 1, 2020

clarkwinkelmann commented Jun 28, 2020

askvortsov1 commented Jun 28, 2020

w-4 Jul 8, 2020

Choose a reason for hiding this comment

clarkwinkelmann commented Jul 28, 2020

askvortsov1 commented May 10, 2020 •

edited

Loading