Page MenuHomePhabricator
Differential D3194

[web] [refactor] currentModal --> modal
ClosedPublic
Actions

Authored by • benschac on Feb 14 2022, 9:35 AM.
Tags
None
Referenced Files
F3245584: D3194.diff
Thu, Nov 14, 7:29 PM
Unknown Object (File)
Thu, Nov 7, 3:20 PM
Unknown Object (File)
Thu, Oct 17, 9:14 PM
Unknown Object (File)
Thu, Oct 17, 9:14 PM
Unknown Object (File)
Thu, Oct 17, 9:14 PM
Unknown Object (File)
Thu, Oct 17, 9:14 PM
Unknown Object (File)
Thu, Oct 17, 9:12 PM
Unknown Object (File)
Sep 20 2024, 6:14 AM
View All Files
Subscribers
• adrian
ashoat
• karol
tomek

Details

Reviewers
atul
ashoat
Commits
rCOMMfbe6c326de48: [web] [refactor] currentModal --> modal
Summary

Part of moving modal state to a context and hook. There is only ever one modal shown at a time.

The rest of the currentModal props will be removed in upcoming diffs when the new hook is applied/used.

Test Plan

Make sure modals still work

Diff Detail

Repository
rCOMM Comm
Branch
lastActiveTimework
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• benschac created this revision.Feb 14 2022, 9:35 AM
Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptFeb 14 2022, 9:35 AM
• benschac edited the summary of this revision. (Show Details)Feb 14 2022, 9:36 AM
Harbormaster completed remote builds in B6801: Diff 9634.Feb 14 2022, 9:40 AM
• benschac requested review of this revision.Feb 14 2022, 9:40 AM
atul added a comment.Feb 14 2022, 12:37 PM

There is only ever one modal shown at a time.

Is it possible that this assumption may change in the future? Will the changes we plan on making affect that possibility?

atul accepted this revision.Feb 14 2022, 12:37 PM
This revision is now accepted and ready to land.Feb 14 2022, 12:37 PM
Herald added a reviewer: ashoat. · View Herald TranscriptFeb 14 2022, 12:37 PM
This revision now requires review to proceed.Feb 14 2022, 12:37 PM
ashoat accepted this revision.Feb 14 2022, 9:35 PM
This revision is now accepted and ready to land.Feb 14 2022, 9:35 PM
• benschac updated this revision to Diff 9666.Feb 15 2022, 6:02 AM

update

Harbormaster completed remote builds in B6827: Diff 9666.Feb 15 2022, 6:07 AM
• benschac added a comment.Feb 15 2022, 6:11 AM
In D3194#85198, @atul wrote:

There is only ever one modal shown at a time.

Is it possible that this assumption may change in the future? Will the changes we plan on making affect that possibility?

Having two modals shown at once is kinda weird UX. I haven't seen it in a while. I'm open to changing it if you think it could change.

At that point maybe second modal would be better?

Closed by commit rCOMMfbe6c326de48: [web] [refactor] currentModal --> modal (authored by • benschac). · Explain WhyFeb 15 2022, 7:09 AM
This revision was automatically updated to reflect the committed changes.
• benschac added a commit: rCOMMfbe6c326de48: [web] [refactor] currentModal --> modal.

Revision Contents
Changeset List

PathSize
web/
13 lines

Diff 9634

View Options

web/app.react.js

Loading...