Page MenuHomePhabricator

[web] [feat] [ENG-716] add icon to modal header
ClosedPublic

Authored by benschac on Apr 4 2022, 2:03 PM.
Tags
None
Referenced Files
F3358188: D3615.diff
Sun, Nov 24, 3:37 AM
Unknown Object (File)
Tue, Nov 19, 3:52 PM
Unknown Object (File)
Tue, Nov 19, 3:52 PM
Unknown Object (File)
Tue, Nov 19, 3:52 PM
Unknown Object (File)
Tue, Nov 19, 3:52 PM
Unknown Object (File)
Mon, Nov 18, 3:08 AM
Unknown Object (File)
Wed, Nov 13, 4:41 AM
Unknown Object (File)
Mon, Nov 11, 11:51 PM

Details

Summary

add icon to modal header, still not styled, just adding the icon per the design

Image 2022-04-04 at 5.00.34 PM.jpg (530×880 px, 40 KB)

https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1060%3A60514
https://linear.app/comm/issue/ENG-716/introduce-leave-channel-modal

delete and leave channel both call for an icon

Test Plan

modal should render with icon when leaving thread

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D3615_2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek added inline comments.
web/modals/modal.react.js
7–8 ↗(On Diff #11021)

I think, these imports could be merged

This revision is now accepted and ready to land.Apr 5 2022, 2:04 AM
web/modals/modal.react.js
7–8 ↗(On Diff #11021)

ty thanks for catching

62 ↗(On Diff #11021)

It's not a ternary, assuming it's okay.

This revision now requires review to proceed.Apr 5 2022, 10:46 AM
tomek added inline comments.
web/modals/modal.react.js
61 ↗(On Diff #11083)

Shouldn't we include span in this conditional rendering?

ashoat requested changes to this revision.Apr 7 2022, 12:06 PM
ashoat added inline comments.
web/modals/modal.react.js
61 ↗(On Diff #11083)

Let's avoid conditional renders in JSX like this. Same reason as our rules for ternary conditions

This revision now requires changes to proceed.Apr 7 2022, 12:06 PM
This revision is now accepted and ready to land.Apr 13 2022, 6:06 AM

If we're setting name to React.Node, then it seems like the parent component can just pass in <><SWMansionIcon size={24} icon={icon} />Name</>, which arguably is cleaner.

(I'd probably rename it from name to header to encapsulate that it contains more than just text.)

@benschac, @palys-swm, @atul – what do you think?

If we're setting name to React.Node, then it seems like the parent component can just pass in <><SWMansionIcon size={24} icon={icon} />Name</>, which arguably is cleaner.

Yeah, that makes sense. I do think having separate props is the better solution here. IE. icon name and content are passed as props separately. Why

  • All headers either have the icon & text or just text in the header. Just two different options. If we had a third option in the modal header I think that would be a different story. If we have a prop that can take any component that's not really enforcing anything.
  • Auto-complete for icon names is a super nice feature that makes the dev experience better.
  • If you're reading the code props, Like
<Modal icon="warning" name="This is the title"

It's just a lot easier to understand than passing in custom JSX each time.

(I'd probably rename it from name to header to encapsulate that it contains more than just text.)

  • That makes sense if we make the change above.