Paths

Table of Contentst

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

	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

	Unknown Object (File)
	Sep 24 2024, 4:24 PM

Subscribers

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

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

Yeah, I can do that.

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.

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.

@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

Path

Size

web/

modals/

2 lines

threads/

confirm-leave-thread-modal.react.js

7 lines

Diff 11919

web/modals/modal.react.js

Loading...

web/modals/threads/confirm-leave-thread-modal.react.js

Loading...