Extract Composer state #2161

askvortsov1 · 2020-05-11T21:24:42Z

See #1821, #2144

Changes proposed in this pull request:

Move state and public API of Composer, ComposerBody (+ inheriting classes), and TextEditor into non-component ComposerState class.
Currently working on getting tags and mentions to work with these changes
It seems that with the BC layer, the Tags and Mentions bundled extensions (which are the ones that most modify the composer/related stuff) are minimally broken, if at all.

Reviewers should focus on:

Should the direct DOM manipulations in ComposerState be moved back into the component classes and linked with events?
Should the opposite be done?
Am I being excessive with use of events?
Should we provide some helper methods as a public api for ComposerState.fields?
How else can we make these changes minimally destructive to extensions? One idea I had was that if app.composer.load is passed a component instance, we could try to manually extract out the class and state, and proceed as usual in a temporary BC layer. The same could be done for modals and alerts.

Confirmed

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

dsevillamartin

Ok, I have a few things to say. Some general comments here, and also a few specific ones to some code.

I think we should have a ComposerState key or something (like my rewrite Alert state) and use that as an attribute or class instead of a generic js-TextEditor because

What's the point of adding js-TextEditor to TextEditor? They both point to the same elements and could target multiple ones
The key will be specific to one and stored in the state, can be generated randomly
- based on Date.now() is probs easiest way
- could use some UUID package or something
- or simply keep track of how many times the state has been constructed and add to the number

We may want a $() method in the ComposerState to return the jQuery (or whatever alternative library) of the corresponding composer DOM.

js/src/forum/components/ReplyComposer.js

js/src/common/utils/subclassOf.js

askvortsov1 · 2020-05-17T23:12:44Z

I think we should have a ComposerState key or something (like my rewrite Alert state) and use that as an attribute or class instead of a generic js-TextEditor because

I wouldn't be opposed to this, but iirc, the concern was adding unnecessary bloat.

What's the point of adding js-TextEditor to TextEditor? They both point to the same elements and could target multiple ones

The goal is to make it clear that js-TextEditor concerns javascript/functionality, not style. So someone could feasibly make changes to / remove / use elsewhere the TextEditor class, but not so much for TextEditor. I would probably prefer a TextEditor-u8dsa7f98a-esque ID though, as you suggest above with your comment about the key. @clarkwinkelmann iirc you didn't like the key idea, what are your thoughts on this?

js/src/forum/ForumApplication.js

askvortsov1 · 2020-05-28T21:20:27Z

Should we wrap bodyClass and props within a JS object like we're doing with alerts and modals?

dsevillamartin · 2020-05-28T21:30:03Z

Should we wrap bodyClass and props within a JS object like we're doing with alerts and modals?

Can you elaborate? Not sure what you mean here.

askvortsov1 · 2020-05-28T21:46:54Z

Well, we wrapped modalClass and modalAttrs into a ModalState class? Perhaps we should do something similar here for consistency so that composers are shown by passing in a ComposerBodyState? (but with different naming)

js/src/forum/components/CommentPost.js

franzliedke · 2020-06-03T23:53:41Z

That .js-TextEditor worries me, too - not so much because of the naming (that .js- convention is a pretty wide-spread convention with the meaning that @askvortsov1 explained), but rather because we're reaching across all kinds of boundaries to a DOM element owned by a completely different part of our app.

Unfortunately, I have no particularly good alternative in mind right now. The problem with JavaScript UIs is that there are so many different types of states. VDOM state (components and props), animation state, actual DOM state, cursor/focus state etc.

And for some of these, it doesn't seem necessary to keep VDOM and DOM state in sync at all times, e.g. when the user is typing lots of words in the composer.

Not sure where I'm going with this...

askvortsov1 · 2020-06-07T00:13:11Z

The best alternative idea I have for the js-TextEditor is providing a unique ID. That would make ownership clearer?

dsevillamartin · 2020-06-07T11:16:06Z

Can we not just use the ref attribute and set it to a state property? Something like ref={el => this.state.el = el}? (I can't remember if this is exactly how it works)

askvortsov1 · 2020-06-10T01:14:09Z

Can we not just use the ref attribute and set it to a state property? Something like ref={el => this.state.el = el}? (I can't remember if this is exactly how it works)

I think this might be a react/vue thing, doesn't seem to do anything. We could, however, pass the texteditor dom element to the state in the config method?

dsevillamartin · 2020-06-10T15:38:06Z

@askvortsov1 It's used for this.element - may have written the wrong code in my comment.

EDIT: Nevermind, it's not used there. I swear I've used it in the past though...

We can still set it using the config attribute.

clarkwinkelmann

I haven't read in full detail, as it's a massive diff.

Globally I think I'm ok with the approach.

Regarding the .js- class stuff, I think that way of doing it is ok. I would even advocate we use an id to make it clear we expect only one of those elements to exist at any given time.

Below a few other things I noticed.

js/src/forum/components/ComposerBody.js

js/src/forum/components/DiscussionComposer.js

js/src/forum/components/ComposerBody.js

js/src/forum/states/ComposerState.js

Since it's a global handler, we should really turn it off when the composer is unloaded. In addition, listening to the event is recommended over setting the global property on `window`.

Brought up by Clark, and simply not working the way it was.

- The main breakthrough was moving the logic from Composer to ComposerBody. The former had to consult the state for logic and values readily available inside the ComposerBody (and its subclasses). - The state is no longer involved in the browser-level unload event handling. - It still has to take care of the manual confirmation message that is shown when the composer itself is closed while drafting a message. - The former does not need a custom message (it would ignore it anyway), the latter does. - The callback and the text that should be displayed are now passed to the state with a nicely encapsulating method. - While I was at it, I extracted a simple helper component for handling the event (de)registration. JSX composition is so nice!

This restores previous behavior. While all ComposerBody subclasses I know of provide their own default values for the `confirmExit` prop, this is technically not a given for other subclasses (e.g. in extensions).

This is no longer a component like it used to, but it is nicely separated from the rest of the state stuff, which only needs to hold (and reset) a reference. The class encapsulates the complex DOM logic for manipulating the contents of a <textarea> HTML tag. The methods are not just moved, but also simplified a bit, as no more conditionals about the existence of the textarea element are needed any longer. We'll see how that goes.

Based on what mentions and emoji were re-implementing themselves.

@w-4

This makes it easier to determine the current responsive screen mode in JavaScript, without hardcoding (and therefore duplicating) breakpoints or CSS properties. Thanks to @w-4 for the great idea!

franzliedke

This is now in a state where I can live with the remaining hacks and rough edges - I've noted down some of them and might try to tackle them at another time. But it's time to unblock the next step, which is upgrading to Mithril 2, and that (not storing component instances) has been achieved for a while.

Sorry for blocking that for so long.

These have been taken care of.

…et('stream')

Regression from #2161.

* 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>

This is needed to have access to the newly created SuperTextarea instance (app.composer.editor) directly after calling show(). Discovered when making ext-mentions work with the Composer state changes. As far as I could reconstruct, a synchronous redraw was also triggered in this situation before the changes in #2161.

* 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>

This is needed to have access to the newly created SuperTextarea instance (app.composer.editor) directly after calling show(). Discovered when making ext-mentions work with the Composer state changes. As far as I could reconstruct, a synchronous redraw was also triggered in this situation before the changes in #2161.

askvortsov1 marked this pull request as ready for review May 11, 2020 21:43

askvortsov1 requested review from dsevillamartin, clarkwinkelmann and tankerkiller125 May 11, 2020 21:43

askvortsov1 mentioned this pull request May 11, 2020

Frontend Framework Rewrite #1821

Closed

65 tasks

dsevillamartin reviewed May 17, 2020

View reviewed changes

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

js/src/common/utils/subclassOf.js Outdated Show resolved Hide resolved

askvortsov1 commented May 28, 2020

View reviewed changes

js/src/forum/ForumApplication.js Show resolved Hide resolved

franzliedke reviewed May 28, 2020

View reviewed changes

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

clarkwinkelmann previously requested changes Jun 16, 2020

View reviewed changes

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

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

askvortsov1 force-pushed the as/frontend-rewrite-composer-state branch 2 times, most recently from 52802c6 to 45eac47 Compare June 19, 2020 00:14

franzliedke reviewed Jun 19, 2020

View reviewed changes

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

askvortsov1 force-pushed the as/frontend-rewrite-composer-state branch from 5ae0366 to e5e2d4f Compare June 19, 2020 22:43

askvortsov1 force-pushed the as/frontend-rewrite-composer-state branch from f11821a to 9155037 Compare June 27, 2020 23:01

franzliedke force-pushed the as/frontend-rewrite-composer-state branch from 9155037 to 9b75ab9 Compare July 4, 2020 19:51

w-4 reviewed Jul 14, 2020

View reviewed changes

js/src/forum/states/ComposerState.js Outdated Show resolved Hide resolved

askvortsov1 added 5 commits July 17, 2020 14:57

Extract ComposerState

4ab390a

Add in small BC layer to slightly reduce chaos

ce17a9d

Slightly expand BC layer to work with mentions

fd80ad9

Encapsulate body subclass in composer state

73f2975

Move bodyClass and bodyProps out into a POJO

889135b

franzliedke added 5 commits July 17, 2020 14:57

Early return for consistency

b2e1fc6

Fix typo

2f6712a

Do not leak onbeforeunload handler

ddbaabc

Since it's a global handler, we should really turn it off when the composer is unloaded. In addition, listening to the event is recommended over setting the global property on `window`.

Fix and simplify discussion title initialization code

9429fb2

Brought up by Clark, and simply not working the way it was.

franzliedke force-pushed the as/frontend-rewrite-composer-state branch from 2a96de6 to 050081c Compare July 17, 2020 14:54

franzliedke added 10 commits July 17, 2020 17:00

Only ask for confirmation when we have a question

a13abf7

This restores previous behavior. While all ComposerBody subclasses I know of provide their own default values for the `confirmExit` prop, this is technically not a given for other subclasses (e.g. in extensions).

Stop using deprecated fields

8fed2d7

Rename method for more expressiveness

7beb807

Fix typehint

553afa1

Encapsulation

9909dd6

SuperTextarea: Add more useful methods for extensions

33535a5

Based on what mentions and emoji were re-implementing themselves.

FIX: DiscussionComposer should not have a "Jump to preview" button

c8e78df

Expose responsive screen mode as CSS variable

c60bc91

This makes it easier to determine the current responsive screen mode in JavaScript, without hardcoding (and therefore duplicating) breakpoints or CSS properties. Thanks to @w-4 for the great idea!

Avoid hardcoding the mobile breakpoint

96aae3d

franzliedke approved these changes Jul 24, 2020

View reviewed changes

Reference correct css variable

7667761

askvortsov1 force-pushed the as/frontend-rewrite-composer-state branch from d7d7db5 to 7667761 Compare July 24, 2020 17:13

Fix leftover reference to app.current.stream (should be app.current.g…

3407dcb

…et('stream')

franzliedke merged commit 5e465f6 into master Jul 24, 2020

franzliedke deleted the as/frontend-rewrite-composer-state branch July 24, 2020 22:17

franzliedke mentioned this pull request Jul 24, 2020

Beta.14 Upgrade Guide #2201

Closed

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

Fix closing the composer with ESC key

64f9fbb

Regression from #2161.

askvortsov1 mentioned this pull request Oct 3, 2020

Cannot read property 'content' of null causes timestamp on post to duplicate for a few seconds #1087

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Composer state #2161

Extract Composer state #2161

askvortsov1 commented May 11, 2020 •

edited by dsevillamartin

Loading

dsevillamartin left a comment •

edited

Loading

askvortsov1 commented May 17, 2020

askvortsov1 commented May 28, 2020

dsevillamartin commented May 28, 2020

askvortsov1 commented May 28, 2020

franzliedke commented Jun 3, 2020

askvortsov1 commented Jun 7, 2020

dsevillamartin commented Jun 7, 2020

askvortsov1 commented Jun 10, 2020

dsevillamartin commented Jun 10, 2020 •

edited

Loading

clarkwinkelmann left a comment

franzliedke left a comment

Extract Composer state #2161

Extract Composer state #2161

Conversation

askvortsov1 commented May 11, 2020 • edited by dsevillamartin Loading

dsevillamartin left a comment • edited Loading

Choose a reason for hiding this comment

askvortsov1 commented May 17, 2020

askvortsov1 commented May 28, 2020

dsevillamartin commented May 28, 2020

askvortsov1 commented May 28, 2020

franzliedke commented Jun 3, 2020

askvortsov1 commented Jun 7, 2020

dsevillamartin commented Jun 7, 2020

askvortsov1 commented Jun 10, 2020

dsevillamartin commented Jun 10, 2020 • edited Loading

clarkwinkelmann left a comment

Choose a reason for hiding this comment

franzliedke left a comment

Choose a reason for hiding this comment

askvortsov1 commented May 11, 2020 •

edited by dsevillamartin

Loading

dsevillamartin left a comment •

edited

Loading

dsevillamartin commented Jun 10, 2020 •

edited

Loading