Extract modal state #2162

askvortsov1 · 2020-05-12T02:01:33Z

Fixes Part of #1821, #2144

Changes proposed in this pull request:

Move modal state out from modal manager component

Reviewers should focus on:

Anything I missed?

Confirmed

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

clarkwinkelmann

A few observations from a quick reading.

Something additional: I would suggest already renaming every "prop" to "attrs" in the new and renamed classes to prepare for Mithril new naming. (like modalProps, directly name it modalAttrs). Not sure what @datitisev things about this. This would also apply to the other PRs

js/src/common/states/ModalState.js

js/src/common/components/Modal.js

askvortsov1 · 2020-05-23T09:11:10Z

Should app.modal.show require a ModalState object? It would be more in line with what we're doing for Alerts, but would also require a bunch more imports...

dsevillamartin · 2020-05-23T13:16:05Z

Not sure if I like having getClass and getAttrs methods (instead of directly accessing them), but we could perhaps add a method to the ModalState to check for the class prototype instance of (as it's used in a few places).

clarkwinkelmann

I'm ok for going ahead with this version of the PR.

I think we should regard ModalState as internal/private API for now. We can still rework the internal later if necessary.

askvortsov1 · 2020-06-17T01:57:27Z

In later versions, I anticipate "ModalState" staying relatively constant, just that it wouldn't be stored in "ModalManagerState". That being said, the only change I'd make at this point is rename it to just "Modal" and put it in an instances directory (or some other naming) as per Franz's comment, don't have the link to that though.

js/src/common/states/ModalManagerState.js

js/src/common/states/ModalState.js

This is a bit more obvious, and more consistent with what we've done for alerts.

- No more weird events - State only has show() and close() - The ModalManager handles Bootstrap's DOM events and signals corresponding state changes to the Modal component and the state. - The Modal component triggers the animations at the right time using Mithril's lifecycle hooks. TODO: - Remove debug output - Ensure that calling close() followed by show() works correctly - Document props

js/src/common/components/Modal.js

js/src/common/states/ModalManagerState.js

Refs flarum/framework#2162.

Regression from #2162.

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

Refs flarum/framework#2162.

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

Refs flarum/framework#2162.

askvortsov1 requested review from clarkwinkelmann and dsevillamartin May 12, 2020 02:01

askvortsov1 mentioned this pull request May 12, 2020

Frontend Framework Rewrite #1821

Closed

65 tasks

clarkwinkelmann requested changes May 15, 2020

View reviewed changes

js/src/common/states/ModalState.js Outdated Show resolved Hide resolved

js/src/common/states/ModalState.js Outdated Show resolved Hide resolved

js/src/common/components/Modal.js Outdated Show resolved Hide resolved

askvortsov1 force-pushed the as/frontend-rewrite-modal-state branch from 2952c26 to 96258f2 Compare May 16, 2020 01:35

askvortsov1 requested a review from clarkwinkelmann May 16, 2020 05:01

clarkwinkelmann approved these changes Jun 16, 2020

View reviewed changes

franzliedke reviewed Jun 19, 2020

View reviewed changes

js/src/common/states/ModalManagerState.js Show resolved Hide resolved

franzliedke reviewed Jun 19, 2020

View reviewed changes

js/src/common/states/ModalState.js Outdated Show resolved Hide resolved

askvortsov1 force-pushed the as/frontend-rewrite-modal-state branch from 96b3730 to cc5c4ce Compare June 19, 2020 21:46

askvortsov1 and others added 11 commits June 26, 2020 18:58

Extract modal state

57825a9

Remove retain stuff

9bf821d

Rename ModalState to ModalManagerState

53ffc4f

Use events for modal component onHide

7bc80ff

Improved how events are used for onhide

ff92ff1

Encapsulate modal state in modal state

df86f88

Fix getClass being recursive for some reason...

b11bf79

WIP encapsulation

fd555e8

Remove obsolete methods that have moved to the state

6c925ec

Pass in props from ModalManager to Modal

29e9993

This is a bit more obvious, and more consistent with what we've done for alerts.

Replace ModalState class with simple PORO

75728f5

franzliedke force-pushed the as/frontend-rewrite-modal-state branch from cc5c4ce to 75728f5 Compare June 26, 2020 17:19

franzliedke force-pushed the as/frontend-rewrite-modal-state branch from e515714 to 1cc02e9 Compare June 26, 2020 22:45

askvortsov1 commented Jun 26, 2020

View reviewed changes

js/src/common/components/Modal.js Outdated Show resolved Hide resolved

askvortsov1 added 2 commits June 27, 2020 18:29

Use static property instead of method for modal isDismissible

680dd3a

Minor cleanup

7e9c94a

clarkwinkelmann requested changes Jun 28, 2020

View reviewed changes

js/src/common/states/ModalManagerState.js Outdated Show resolved Hide resolved

askvortsov1 and others added 3 commits June 30, 2020 18:13

Merge branch 'master' into as/frontend-rewrite-modal-state

49f61e5

Rename type to component class, add breaking change compliance warning

96e4496

prettier doing it's thing

0263295

askvortsov1 merged commit 44376ce into master Jun 30, 2020

askvortsov1 deleted the as/frontend-rewrite-modal-state branch June 30, 2020 23:59

franzliedke mentioned this pull request Jul 1, 2020

Beta.14 Upgrade Guide #2201

Closed

askvortsov1 added the breaking-changes label Jul 1, 2020

franzliedke added a commit to flarum/emoji that referenced this pull request Jul 24, 2020

Fix extension to work with Composer state changes

d77df41

Refs flarum/framework#2162.

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

Remove obsolete method

ce3d08c

Regression from #2162.

franzliedke added a commit to flarum/mentions that referenced this pull request Sep 4, 2020

Fix extension to work with Composer state changes

ce0afdc

Refs flarum/framework#2162.

franzliedke added a commit to flarum/mentions that referenced this pull request Sep 4, 2020

Fix extension to work with Composer state changes

37bad84

Refs flarum/framework#2162.

franzliedke added a commit to flarum/mentions that referenced this pull request Sep 4, 2020

Fix extension to work with Composer state changes

6fc5498

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/emoji that referenced this pull request Mar 11, 2022

Fix extension to work with Composer state changes

3944a0f

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/mentions that referenced this pull request Mar 11, 2022

Fix extension to work with Composer state changes

28263cb

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/mentions that referenced this pull request Mar 11, 2022

Fix extension to work with Composer state changes

d3f0f8b

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/mentions that referenced this pull request Mar 11, 2022

Fix extension to work with Composer state changes

dc43c22

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/emoji that referenced this pull request May 10, 2022

Fix extension to work with Composer state changes

8840418

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/mentions that referenced this pull request May 10, 2022

Fix extension to work with Composer state changes

b1ee616

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/mentions that referenced this pull request May 10, 2022

Fix extension to work with Composer state changes

7cc0075

Refs flarum/framework#2162.

askvortsov1 pushed a commit to flarum/mentions that referenced this pull request May 10, 2022

Fix extension to work with Composer state changes

d75a8ca

Refs flarum/framework#2162.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract modal state #2162

Extract modal state #2162

askvortsov1 commented May 12, 2020

clarkwinkelmann left a comment

askvortsov1 commented May 23, 2020

dsevillamartin commented May 23, 2020

clarkwinkelmann left a comment

askvortsov1 commented Jun 17, 2020

Extract modal state #2162

Extract modal state #2162

Conversation

askvortsov1 commented May 12, 2020

clarkwinkelmann left a comment

Choose a reason for hiding this comment

askvortsov1 commented May 23, 2020

dsevillamartin commented May 23, 2020

clarkwinkelmann left a comment

Choose a reason for hiding this comment

askvortsov1 commented Jun 17, 2020