feat(modals): support stacking modals, remove bootstrap modals dependency #3456

jaspervriends · 2022-06-10T23:23:10Z

Changes proposed in this pull request:
In this PR I've worked on making it possible to have multiple dialogs opened at the same time and make them stackable. I have partly used code of #3246 and altered it. It also removed the bootstrap modal dependency.

However, I did not proceed using the native HTML Dialog element for this PR due to lack of animations (for example, the backdrop is not animatable) and responsiveness with the interface.

A third parameter has been added to the app.modal.show function which is a boolean and defaults to false. This means that all already existing modals will keep their intended behaviour.

I also copied the new 'dismissable' options from the other PR, such as an option to hide the dialog on escape key, clicking on the backdrop or using the close button and managing them seperately per modal. This means there is a deprecation too which was mentioned in the code.

Reviewers should focus on:

Current modals are opening and closing as intended
Stacking multiple modals works
Responsiveness is still good

Necessity

Has the problem that is being solved here been clearly explained?
If applicable, have various options for solving this problem been considered?
For core PRs, does this need to be in core, or could it be in an extension?
Are we willing to maintain this for years / potentially forever?

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).
Core developer confirmed locally this works as intended.
Tests have been added, or are not appropriate here.

davwheat

Just some comments so far. Haven't had time to properly review yet.

framework/core/js/src/common/components/ModalManager.tsx

framework/core/js/src/common/states/ModalManagerState.ts

framework/core/js/src/common/components/ModalManager.tsx

jaspervriends · 2022-06-15T18:57:17Z

Thanks for the feedback, will work on it later this week

jaspervriends · 2022-06-26T14:57:25Z

Processed feedback :)

davwheat

I've addressed some things with commits on this PR, but would like a sanity check for my changes from another core dev before looking to merge this.

Thank you for your work on this! This is a big improvement for Flarum's modals system! :)

SychO9

Code looks good! a few comments and a bug I noticed. Thanks a lot for the PR!

There's an issue with the scrolling behavior, I think a video will better explain it:
https://user-images.githubusercontent.com/20267363/178266180-69aaa829-b528-4808-999f-4693bd15b3a5.mp4

framework/core/js/src/forum/components/LogInModal.tsx

framework/core/less/common/Modal.less

framework/core/js/src/common/states/ModalManagerState.ts

davwheat · 2022-07-14T11:10:21Z

oops... that was the wrong git command...

have asked jasper for write access to his fork so i can resurrect this. have a backup of the code at https://github.com/flarum/framework/tree/jv/stackable-modals

davwheat · 2022-07-14T13:11:20Z

RE the scrolling bug: isn't this how the modals currently work in core?

uoKyaMOF.mp4

SychO9 · 2022-07-14T13:14:49Z

yea but that isn't how they work in this branch 🙈 0:15 of my uploaded video shows behavior in this branch

davwheat · 2022-07-14T13:36:53Z

OH I didn't watch for long enough! Right, I'll get on that.

davwheat · 2022-07-14T14:53:26Z

@SychO9 Fixed, I hope.

SychO9 · 2022-07-15T14:03:32Z

🙈 now you can't scroll inside a modal

New changes

SychO9

Using one backdrop seems great! I think only small tweaks are left.

framework/core/less/common/Modal.less

framework/core/js/src/common/components/ModalManager.tsx

framework/core/less/common/Modal.less

luceos · 2022-07-29T09:12:44Z

The patience in this PR is admirable.

Require explicit values for `attrs` and `stackModal` from Flarum 2.0, beginning deprecation from now.

Co-authored-by: Sami Mazouz <sychocouldy@gmail.com>

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

davwheat · 2022-08-08T12:27:14Z

Rebased on main.

davwheat · 2022-08-08T12:29:25Z

phew that was a big one 😄

davwheat reviewed Jun 11, 2022

View reviewed changes

SychO9 mentioned this pull request Jun 20, 2022

feat: add tag selector custom setting component for buildSettingComponent #3495

Closed

9 tasks

jaspervriends requested a review from davwheat June 26, 2022 14:57

davwheat requested a review from SychO9 July 11, 2022 07:39

davwheat previously approved these changes Jul 11, 2022

View reviewed changes

davwheat changed the title ~~Feature: Stackable modals and removed bootstrap dialogs~~ feat(modals): support stacking modals, remove bootstrap modals dependency Jul 11, 2022

davwheat self-assigned this Jul 11, 2022

davwheat added type/feature type/cleanup labels Jul 11, 2022

davwheat added this to the 1.5 milestone Jul 11, 2022

davwheat added the type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) label Jul 11, 2022

SychO9 previously requested changes Jul 11, 2022

View reviewed changes

framework/core/js/src/forum/components/LogInModal.tsx Outdated Show resolved Hide resolved

framework/core/less/common/Modal.less Outdated Show resolved Hide resolved

framework/core/js/src/common/states/ModalManagerState.ts Outdated Show resolved Hide resolved

davwheat closed this Jul 14, 2022

davwheat force-pushed the main branch from 3f2bf06 to 16f59f5 Compare July 14, 2022 11:09

davwheat reopened this Jul 14, 2022

davwheat requested a review from SychO9 July 14, 2022 13:09

luceos self-requested a review July 25, 2022 09:02

SychO9 reviewed Jul 26, 2022

View reviewed changes

framework/core/less/common/Modal.less Outdated Show resolved Hide resolved

framework/core/js/src/common/components/ModalManager.tsx Show resolved Hide resolved

framework/core/less/common/Modal.less Outdated Show resolved Hide resolved

davwheat force-pushed the main branch from 2a01e12 to 1e7b11f Compare July 31, 2022 12:46

davwheat and others added 23 commits August 8, 2022 13:26

fix: backdrop flashing as modals are stacked/unstacked

596a9c5

chore: simplify modal close process

0b452d4

docs: add TS overload to indicate deprecated modal opening syntax

163df59

Require explicit values for `attrs` and `stackModal` from Flarum 2.0, beginning deprecation from now.

feat: use stackable modal for forgot password modal above sign in

f1575f1

chore: explicitly check if modal is open before trying to focus trap

ae66967

fix(a11y): add missing aria-hidden on main content when modal open

07ffcbe

fix(a11y): add missing aria-modal on modal

4ef7c04

chore: remove test code

c9fdb8f

chore: remove dead CSS

15845b7

chore: remove overload

c655e4e

fix: lock page scrolling when modal is open

b004ad2

fix: strange scrolling behaviour

27e4aa4

chore: convert to JSX

b000a6a

fix: event listener memory leak

0859f33

chore: remove unneeded optional chaining

9513579

fix: incorrect return types

efd8b43

chore: rewrite backdrop system - use one backdrop for all modals

fd42ac3

docs: typos in comment block

aa52a01

fix: show backdrop directly below top-most modal

377215b

Co-authored-by: Sami Mazouz <sychocouldy@gmail.com>

chore: format

8492c2d

fix: use an invisible backdrop for each modal to exit

9c5a1ff

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

chore: remove debugging code

8c6eac7

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

chore: remove forgotten debug code

4a0a7a4

davwheat force-pushed the main branch from bb7ca0a to 4a0a7a4 Compare August 8, 2022 12:27

SychO9 approved these changes Aug 8, 2022

View reviewed changes

davwheat merged commit f69210b into flarum:main Aug 8, 2022

luceos mentioned this pull request Sep 8, 2022

Release v1.5.0 #3638

Closed

SychO9 added the needs-documentation label Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(modals): support stacking modals, remove bootstrap modals dependency #3456

feat(modals): support stacking modals, remove bootstrap modals dependency #3456

jaspervriends commented Jun 10, 2022 •

edited by davwheat

Loading

davwheat left a comment

jaspervriends commented Jun 15, 2022

jaspervriends commented Jun 26, 2022

davwheat left a comment •

edited

Loading

SychO9 left a comment

davwheat commented Jul 14, 2022 •

edited

Loading

davwheat commented Jul 14, 2022

SychO9 commented Jul 14, 2022

davwheat commented Jul 14, 2022

davwheat commented Jul 14, 2022

SychO9 commented Jul 15, 2022

SychO9 left a comment

luceos commented Jul 29, 2022

davwheat commented Aug 8, 2022

davwheat commented Aug 8, 2022

feat(modals): support stacking modals, remove bootstrap modals dependency #3456

feat(modals): support stacking modals, remove bootstrap modals dependency #3456

Conversation

jaspervriends commented Jun 10, 2022 • edited by davwheat Loading

davwheat left a comment

Choose a reason for hiding this comment

jaspervriends commented Jun 15, 2022

jaspervriends commented Jun 26, 2022

davwheat left a comment • edited Loading

Choose a reason for hiding this comment

SychO9 left a comment

Choose a reason for hiding this comment

davwheat commented Jul 14, 2022 • edited Loading

davwheat commented Jul 14, 2022

SychO9 commented Jul 14, 2022

davwheat commented Jul 14, 2022

davwheat commented Jul 14, 2022

SychO9 commented Jul 15, 2022

SychO9 left a comment

Choose a reason for hiding this comment

luceos commented Jul 29, 2022

davwheat commented Aug 8, 2022

davwheat commented Aug 8, 2022

jaspervriends commented Jun 10, 2022 •

edited by davwheat

Loading

davwheat left a comment •

edited

Loading

davwheat commented Jul 14, 2022 •

edited

Loading