Page MenuHomePhabricator
Differential D3616

[web] [fix] [ENG-716] use cross icon in modal
ClosedPublic
Actions

Authored by • benschac on Apr 4 2022, 2:46 PM.
Tags
None
Referenced Files
F3358703: D3616.diff
Sun, Nov 24, 5:51 AM
Unknown Object (File)
Thu, Nov 21, 9:45 PM
Unknown Object (File)
Thu, Nov 7, 10:25 AM
Unknown Object (File)
Thu, Nov 7, 4:10 AM
Unknown Object (File)
Wed, Nov 6, 10:30 AM
Unknown Object (File)
Wed, Nov 6, 8:26 AM
Unknown Object (File)
Mon, Nov 4, 4:01 PM
Unknown Object (File)
Sun, Nov 3, 3:18 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
• karol
tomek

Details

Reviewers
atul
tomek
• jacek
Commits
rCOMM3a07611abc79: [web] [fix] [ENG-716] use cross icon in modal
Summary

swap x span for SWM icon

https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1061%3A63258 -- some modals have the x button, others don't: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1062%3A64128

x button is now optional

Test Plan

render icon

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• benschac created this revision.Apr 4 2022, 2:46 PM
Herald added subscribers: • abosh, • karol, • adrian and 2 others. · View Herald TranscriptApr 4 2022, 2:46 PM
• benschac updated this revision to Diff 11030.Apr 4 2022, 2:54 PM

add optional x button

• benschac added a parent revision: D3615: [web] [feat] [ENG-716] add icon to modal header.Apr 4 2022, 2:54 PM
• benschac retitled this revision from [web] [fix] use cross icon in modal to [web] [fix] [ENG-716] use cross icon in modal.
• benschac edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B7855: Diff 11029.Apr 4 2022, 3:02 PM
Harbormaster completed remote builds in B7856: Diff 11030.Apr 4 2022, 3:09 PM
• benschac requested review of this revision.Apr 4 2022, 3:09 PM
• benschac added reviewers: tomek, • jacek.Apr 5 2022, 4:35 AM
• benschac added a child revision: D3618: [web] [feat] [ENG-972] update modal header to match design.Apr 5 2022, 8:03 AM
• benschac updated this revision to Diff 11084.Apr 5 2022, 1:32 PM

rebase

Harbormaster completed remote builds in B7905: Diff 11084.Apr 5 2022, 1:58 PM
tomek requested changes to this revision.Apr 6 2022, 7:29 AM
tomek added inline comments.
web/modals/modal.react.js
15 ↗(On Diff #11084)

Why is it a Node? We're checking only if it is present so this can be a boolean.

Also, the naming is confusing. Starting the name with on suggests it's a callback.

This revision now requires changes to proceed.Apr 6 2022, 7:29 AM
• benschac updated this revision to Diff 11183.Apr 7 2022, 8:50 AM

update type

Harbormaster failed remote builds in B7979: Diff 11183!Apr 7 2022, 9:03 AM
tomek requested changes to this revision.Apr 7 2022, 11:24 PM
tomek added inline comments.
web/modals/modal.react.js
15 ↗(On Diff #11084)

What do you think about the second question (naming)?

This revision now requires changes to proceed.Apr 7 2022, 11:24 PM
Harbormaster completed remote builds in B7979: Diff 11183.Apr 8 2022, 8:14 AM
ashoat added a comment.Apr 8 2022, 8:25 AM

Agree with Tomek about naming – onCloseButton sounds like a callback that gets called when a button is closed

web/modals/modal.react.js
56–60 ↗(On Diff #11183)

Breaks ternary rule #3, see here:

Either the whole expression should fit on one line, or the ? and : characters should appear at the start of subsequent lines.

• benschac updated this revision to Diff 11320.Apr 11 2022, 1:17 PM

address diff feedback

• benschac added inline comments.Apr 11 2022, 1:20 PM
web/modals/modal.react.js
15 ↗(On Diff #11084)

would handle be better?

Harbormaster completed remote builds in B8094: Diff 11320.Apr 11 2022, 1:24 PM
tomek requested changes to this revision.Apr 12 2022, 1:37 AM
tomek added inline comments.
web/modals/modal.react.js
15 ↗(On Diff #11084)

It might be a little confusing by suggesting that this modal does something special when handling the close button. It looks like we're deciding if we want to show the close button, so e.g. showCloseButton might be slightly better, but it also sounds like an action. An even better name could be withCloseButton - I really like that because it can be read really well when used e.g. <Modal withCloseButton /> - which means exactly what is said. We can also consider something like isCloseButtonVisible or hasCloseButton.

One idea we can consider is to not introduce this prop at all and instead to make onClose optional. Then, based on presence of this function, decide if we want the button. What do you think?

This revision now requires changes to proceed.Apr 12 2022, 1:37 AM
• benschac updated this revision to Diff 11357.Apr 12 2022, 10:38 AM

use optional onClose

• benschac added a comment.Apr 12 2022, 10:39 AM

I think optional onClose makes the most sense.

Harbormaster completed remote builds in B8128: Diff 11357.Apr 12 2022, 10:46 AM
tomek requested changes to this revision.Apr 13 2022, 12:18 AM
In D3616#102122, @benschac wrote:

I think optional onClose makes the most sense.

Looking at the code, I think my suggestion was wrong... sorry for that. If we make onClose optional and decide to not set it, there would be no way to close the modal, e.g. when clicking outside of it. I think we should reintroduce the boolean prop and make onClose mandatory

This revision now requires changes to proceed.Apr 13 2022, 12:18 AM
• benschac updated this revision to Diff 11444.Apr 13 2022, 11:07 AM

use withCloseButton prop

Harbormaster completed remote builds in B8175: Diff 11444.Apr 13 2022, 11:12 AM
• benschac updated this revision to Diff 11450.Apr 13 2022, 12:12 PM

rebase

Harbormaster completed remote builds in B8181: Diff 11450.Apr 13 2022, 12:20 PM
tomek requested changes to this revision.Apr 14 2022, 1:09 AM

While rebasing, you reintroduced the version with optional onClick

This revision now requires changes to proceed.Apr 14 2022, 1:09 AM
• benschac updated this revision to Diff 11472.Apr 14 2022, 5:25 AM

re add fix

• benschac updated this revision to Diff 11473.Apr 14 2022, 5:27 AM

fix

• benschac planned changes to this revision.Apr 14 2022, 5:27 AM
Harbormaster completed remote builds in B8201: Diff 11472.Apr 14 2022, 5:29 AM
• benschac updated this revision to Diff 11474.Apr 14 2022, 5:32 AM

revert

Harbormaster completed remote builds in B8202: Diff 11473.Apr 14 2022, 5:33 AM
Harbormaster completed remote builds in B8203: Diff 11474.Apr 14 2022, 5:37 AM
tomek accepted this revision.Apr 14 2022, 10:48 AM
This revision is now accepted and ready to land.Apr 14 2022, 10:48 AM
Closed by commit rCOMM3a07611abc79: [web] [fix] [ENG-716] use cross icon in modal (authored by • benschac). · Explain WhyApr 15 2022, 2:12 PM
This revision was automatically updated to reflect the committed changes.
• benschac added a commit: rCOMM3a07611abc79: [web] [fix] [ENG-716] use cross icon in modal.
atul added a comment.Apr 18 2022, 10:08 AM

Looks like this change affected ThreadSettingsModal. For that modal we definitely want to keep the "X" button so the user can leave the modal without having to save changes.

I think we should have either:

A. Enabled the close button by default
B. Gone through the existing modals and made sure that they continue to look as they did prior. Making withCloseButton a mandatory prop may be a good way to surface all of the modals that need to be looked at.

ashoat added a comment.Apr 18 2022, 9:04 PM

@atul, did you create a task to track the regression introduced by this diff? Please always create a task for something that needs to be followed up on, and please always share a link to that task when leaving a comment referencing a required follow-up.

Revision Contents
Changeset List

PathSize
web/
modals/
24 lines

Diff 11320

View Options

web/modals/modal.react.js

Loading...