Page MenuHomePhabricator
Differential D3820

[web] [fix] [ENG-1033] change modal x default to show on default
ClosedPublic
Actions

Authored by • benschac on Apr 22 2022, 2:39 PM.
Tags
None
Referenced Files
F3389145: D3820.diff
Fri, Nov 29, 5:46 PM
Unknown Object (File)
Fri, Nov 8, 10:43 PM
Unknown Object (File)
Tue, Nov 5, 3:45 AM
Unknown Object (File)
Tue, Nov 5, 3:45 AM
Unknown Object (File)
Tue, Nov 5, 3:45 AM
Unknown Object (File)
Tue, Nov 5, 3:45 AM
Unknown Object (File)
Tue, Nov 5, 3:44 AM
Unknown Object (File)
Oct 8 2024, 12:16 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
• karol

Details

Reviewers
atul
tomek
• jacek
Commits
rCOMM1f0272f1e14f: [web] [fix] [ENG-1033] change modal x default to show on default
Summary

re-add the close button

Image 2022-04-22 at 5.30.11 PM.jpg (1×1 px, 87 KB)

Image 2022-04-22 at 5.30.02 PM.jpg (620×1 px, 55 KB)

https://linear.app/comm/issue/ENG-1033/close-button-removed-from-modal

Test Plan

all modals should have the x button by default. attaching screen shots.

Diff Detail

Repository
rCOMM Comm
Branch
benjamin/eng-1033-close-button-removed-from-modal
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• benschac created this revision.Apr 22 2022, 2:39 PM
Herald added subscribers: • abosh, • karol, • adrian, ashoat. · View Herald TranscriptApr 22 2022, 2:39 PM
• benschac edited the summary of this revision. (Show Details)Apr 22 2022, 2:39 PM
• benschac edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B8476: Diff 11819.Apr 22 2022, 2:44 PM
• benschac requested review of this revision.Apr 22 2022, 2:44 PM
atul accepted this revision.Apr 24 2022, 2:27 PM

Should you also go back and explicitly set withCloseButton to false for the modals you decided didn't need a close button?

This revision is now accepted and ready to land.Apr 24 2022, 2:27 PM
• benschac added a comment.Apr 25 2022, 5:25 AM

Yeah, I can do that.

• benschac updated this revision to Diff 11914.Apr 26 2022, 7:19 AM

update leave modal

Harbormaster completed remote builds in B8548: Diff 11914.Apr 26 2022, 7:24 AM
• benschac added inline comments.Apr 26 2022, 7:40 AM
web/modals/threads/confirm-leave-thread-modal.react.js
21 ↗(On Diff #11914)

After looking through the codebase. The modal component is used in 16 places. The only place so far that looks like it doesn't call for an 'x' is the leaving thread modal, per the design.

• benschac added a comment.Apr 26 2022, 7:40 AM
In D3820#106142, @atul wrote:

Should you also go back and explicitly set withCloseButton to false for the modals you decided didn't need a close button?

Addressed this in the inline comment.

• benschac requested review of this revision.Apr 26 2022, 7:41 AM

@atul -- can you take another look?

atul accepted this revision.Apr 26 2022, 7:55 AM
atul added inline comments.
web/modals/threads/confirm-leave-thread-modal.react.js
21 ↗(On Diff #11914)

The only place so far that looks like it doesn't call for an 'x' is the leaving thread modal, per the design.

Makes sense. Personally curious if omitting the close button for this modal was intentional or an oversight... but seems fine to me either way.

This revision is now accepted and ready to land.Apr 26 2022, 7:55 AM
Closed by commit rCOMM1f0272f1e14f: [web] [fix] [ENG-1033] change modal x default to show on default (authored by • benschac). · Explain WhyApr 26 2022, 7:57 AM
This revision was automatically updated to reflect the committed changes.
• benschac added a commit: rCOMM1f0272f1e14f: [web] [fix] [ENG-1033] change modal x default to show on default.

Revision Contents
Changeset List

PathSize
web/
modals/
2 lines

Diff 11819

View Options

web/modals/modal.react.js

Loading...