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
Unknown Object (File)
Sat, Oct 5, 4:38 AM
Unknown Object (File)
Mon, Sep 23, 4:16 AM
Unknown Object (File)
Mon, Sep 23, 4:16 AM
Unknown Object (File)
Mon, Sep 23, 4:16 AM
Unknown Object (File)
Mon, Sep 23, 3:41 AM
Unknown Object (File)
Mon, Sep 23, 3:41 AM
Unknown Object (File)
Mon, Sep 23, 3:41 AM
Unknown Object (File)
Mon, Sep 23, 3:41 AM

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
leave-channel-thread
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek added inline comments.
web/modals/modal.react.js
7–8

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

ty thanks for catching

62

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.