Remove page-based modals in favor of existing state-based modal logic #1270

j-f1 · 2023-07-03T20:25:24Z

netlify · 2023-07-03T20:25:49Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`b8c1f58`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/64d2f7aabc75470008396b9d
😎 Deploy Preview	https://deploy-preview-1270.demo.actualbudget.org/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions · 2023-07-03T20:44:12Z

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
14	2.12 MB → 2.12 MB (-2.01 kB)	-0.09%

Changeset

File	Δ	Size
`src/components/Modals.tsx`	📈 +840 B (+16.07%)	5.11 kB → 5.93 kB
`src/components/accounts/MobileAccountDetails.js`	📈 +30 B (+0.92%)	3.2 kB → 3.23 kB
`src/components/accounts/Header.js`	📈 +30 B (+0.35%)	8.35 kB → 8.38 kB
`src/components/manager/Modals.js`	📈 +3 B (+0.15%)	1.96 kB → 1.96 kB
`src/components/schedules/PostsOfflineNotification.js`	📉 -5 B (-0.22%)	2.25 kB → 2.24 kB
`src/components/schedules/index.js`	📉 -7 B (-0.34%)	2.04 kB → 2.03 kB
`src/components/accounts/Account.js`	📉 -128 B (-0.41%)	30.43 kB → 30.31 kB
`src/global-events.js`	📉 -41 B (-1.09%)	3.66 kB → 3.62 kB
`src/components/transactions/TransactionList.js`	📉 -68 B (-1.30%)	5.11 kB → 5.05 kB
`src/components/schedules/EditSchedule.js`	📉 -308 B (-1.91%)	15.77 kB → 15.47 kB
`src/components/transactions/SelectedTransactions.js`	📉 -69 B (-2.31%)	2.92 kB → 2.85 kB
`src/components/schedules/LinkSchedule.js`	📉 -80 B (-5.10%)	1.53 kB → 1.46 kB
`src/components/schedules/DiscoverSchedules.js`	📉 -356 B (-6.92%)	5.03 kB → 4.68 kB
`src/components/FinancesApp.tsx`	📉 -1.2 kB (-14.50%)	8.25 kB → 7.05 kB
`src/components/Page.tsx`	📉 -721 B (-37.24%)	1.89 kB → 1.19 kB
`src/components/responsive/wide.ts`	📉 -199 B (-43.26%)	460 B → 261 B
`src/util/router-tools.tsx`	📉 -1.41 kB (-87.19%)	1.62 kB → 212 B
`src/components/ActiveLocation.js`	🔥 -396 B (-100%)	396 B → 0 B

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
static/js/main.js	944.09 kB → 961.58 kB (+17.48 kB)	+1.85%
static/js/narrow-components.chunk.js	24.62 kB → 24.64 kB (+23 B)	+0.09%

Smaller

Asset	File Size	% Changed
static/js/wide-components.chunk.js	156.68 kB → 137.17 kB (-19.51 kB)	-12.45%

Unchanged

Asset	File Size	% Changed
static/js/171.chunk.js	396.24 kB	0%
static/media/Inter-italic.var.woff2	239.29 kB	0%
static/media/Inter-roman.var.woff2	221.86 kB	0%
static/js/383.chunk.js	117.35 kB	0%
static/js/reports.chunk.js	40.96 kB	0%
static/js/969.chunk.js	12.94 kB	0%
static/js/resize-observer-polyfill.chunk.js	8.12 kB	0%
static/css/main.css	5.82 kB	0%
asset-manifest.json	1.78 kB	0%
index.html	1.66 kB	0%
static/media/browser-server.js	963 B	0%

github-actions · 2023-07-03T20:44:14Z

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
2	1.97 MB	0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
kcab.worker.js	1012.13 kB	0%
xfo.xfo.kcab.worker.js	1004.04 kB	0%

MatissJanis · 2023-07-28T20:38:54Z

Can you explain this a bit more? I think I'm missing some context so I don't understand why this change is necessary and what problem it would solve.

j-f1 · 2023-07-28T21:04:08Z

Right now, we have two parallel systems for displaying modals on top of the current page. One of them uses an array of modals stored in Redux state, then manually renders those modals in an overlay. The other one uses a linked list of modals stored as the current location, with the locationPtr state object pointing to the parent modal (or the root page object).

While the location-based modals offer some advantages (for example, they update the URL when invoked, and we can use React Router's matching logic to select which modals to display, it requires a few hacks:

The location object itself is stored in the location state in order to be restored when the modal is closed (rather than some sort of representation of the URL the location corresponds to)
Reloading the page transforms the modal into a full page, which has undesirable results in most cases because the modal was not designed to integrate well as a page. So we need to manually redirect back to a sensible page. Meanwhile, the Redux based modals just disappear when reloading (or we could theoretically save them) but the underlying page remains the same.
Similar to 2, the sidebar and any other components that use the current URL to change appearance behave incorrectly when a modal is opened because they begin to correspond to the modal, not the page they're visually adjacent to.
React Router is trying to move users to new APIs, and the model where we explicitly pass in a custom location object in order to render the page or modals underneath the frontmost modal would not work as is.
It's a bit confusing — where should new modals be added? Now there is only one place.
Technically, any page could be rendered as a modal. I think it's best to have to explicitly opt in components to being rendered as a modal, and explicitly update them to handle that. Most components should probably be either always a page or always a modal.
As you can see in the PR, the current implementation overloads <Page /> to also represent modals which is IMO a bad mismatch.

MatissJanis · 2023-07-29T10:11:20Z

Thanks for the explanation.

Yes, it makes sense to port everything over to a single system. Thanks for taking this up! I'll do a review once all the CI jobs pass.

j-f1 · 2023-07-29T14:38:44Z

Fixed the CI!

MatissJanis

Would appreciate a review from @trevdor too since he's our react-router expert.

packages/desktop-client/src/components/schedules/LinkSchedule.js

packages/desktop-client/src/components/schedules/index.js

MatissJanis

LGTM on my end

trevdor · 2023-08-09T00:47:01Z

🦥 I'm slow to get to this, but aim to look at it later tonight.

trevdor

SO much easier to reason about when all modals follow a single paradigm. Thank you!

…actualbudget#1270) Co-authored-by: Trevor Farlow <[email protected]>

trafico-bot bot added the 🔍 Ready for Review label Jul 3, 2023

j-f1 force-pushed the jed/no-url-modals branch 2 times, most recently from a415ab5 to 5578f18 Compare July 3, 2023 20:34

j-f1 marked this pull request as draft July 3, 2023 20:40

j-f1 force-pushed the jed/no-url-modals branch 2 times, most recently from 4230169 to 2b03108 Compare July 4, 2023 13:17

j-f1 added 8 commits July 20, 2023 08:30

Remove page-based modals in favor of existing state-based modal logic

8397b19

Remove unnecessary StackedRoutesInner

e784b97

Fix build issues

fba0a88

More fixing

092ad59

Fix closing modals

b9939b4

Fix lint error

2595591

Add release note

29e60e6

Remove unused initialFields option on DiscoverSchedules

c4f283c

j-f1 force-pushed the jed/no-url-modals branch from b27ee57 to ba8ede5 Compare July 20, 2023 12:39

fix another lint error

d263f97

j-f1 force-pushed the jed/no-url-modals branch from 57625d4 to d263f97 Compare July 21, 2023 12:57

Default modal options to {}

1992cde

j-f1 marked this pull request as ready for review July 21, 2023 13:23

j-f1 mentioned this pull request Jul 21, 2023

Refactor Rules Editor page to copy Schedules and Filters modals #1162

Closed

2 tasks

Merge branch 'master' into jed/no-url-modals

00e90bf

Merge branch 'master' into jed/no-url-modals

be9b974

j-f1 added 2 commits July 29, 2023 04:58

Merge branch 'master' into jed/no-url-modals

26bf444

Merge branch 'master' into jed/no-url-modals

44d0417

j-f1 added 3 commits July 29, 2023 06:17

Fix bad merge

9e1d533

Default modal options to {}

92a09d6

Fix a different way

4a10bad

MatissJanis requested changes Jul 29, 2023

View reviewed changes

packages/desktop-client/src/components/schedules/LinkSchedule.js Outdated Show resolved Hide resolved

packages/desktop-client/src/components/schedules/index.js Outdated Show resolved Hide resolved

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review labels Jul 29, 2023

j-f1 added 2 commits July 29, 2023 17:17

;

97f82d3

Remove connect()

aed3203

trafico-bot bot added 🔍 Ready for Review and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Jul 30, 2023

j-f1 requested a review from trevdor July 30, 2023 00:44

MatissJanis approved these changes Jul 30, 2023

View reviewed changes

trafico-bot bot added ✅ Approved and removed 🔍 Ready for Review labels Jul 30, 2023

Merge branch 'master' into jed/no-url-modals

17cde77

trafico-bot bot added 🔍 Ready for Review and removed ✅ Approved labels Jul 30, 2023

j-f1 added 2 commits July 30, 2023 14:59

Merge branch 'master' into jed/no-url-modals

e67741c

Fix bad import

70a22c7

Merge remote-tracking branch 'origin/master' into jed/no-url-modals

b8c1f58

trevdor approved these changes Aug 9, 2023

View reviewed changes

trafico-bot bot added ✅ Approved and removed 🔍 Ready for Review labels Aug 9, 2023

trevdor merged commit c667118 into master Aug 9, 2023
15 checks passed

trevdor deleted the jed/no-url-modals branch August 9, 2023 02:35

trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Aug 9, 2023

FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024

Remove page-based modals in favor of existing state-based modal logic (…

8b7e2e3

…actualbudget#1270) Co-authored-by: Trevor Farlow <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove page-based modals in favor of existing state-based modal logic #1270

Remove page-based modals in favor of existing state-based modal logic #1270

j-f1 commented Jul 3, 2023

netlify bot commented Jul 3, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading

MatissJanis commented Jul 28, 2023

j-f1 commented Jul 28, 2023

MatissJanis commented Jul 29, 2023

j-f1 commented Jul 29, 2023

MatissJanis left a comment

MatissJanis left a comment

trevdor commented Aug 9, 2023

trevdor left a comment

Remove page-based modals in favor of existing state-based modal logic #1270

Remove page-based modals in favor of existing state-based modal logic #1270

Conversation

j-f1 commented Jul 3, 2023

netlify bot commented Jul 3, 2023 • edited Loading

✅ Deploy Preview for actualbudget ready!

github-actions bot commented Jul 3, 2023 • edited Loading

Bundle Stats — desktop-client

github-actions bot commented Jul 3, 2023 • edited Loading

Bundle Stats — loot-core

MatissJanis commented Jul 28, 2023

j-f1 commented Jul 28, 2023

MatissJanis commented Jul 29, 2023

j-f1 commented Jul 29, 2023

MatissJanis left a comment

Choose a reason for hiding this comment

MatissJanis left a comment

Choose a reason for hiding this comment

trevdor commented Aug 9, 2023

trevdor left a comment

Choose a reason for hiding this comment

netlify bot commented Jul 3, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading