Page MenuHomePhabricator
Differential D8479

[web] Update the change role icon on web
ClosedPublic
Actions

Authored by rohan on Jul 11 2023, 3:08 PM.
Tags
None
Referenced Files
F2905113: D8479.id28638.diff
Sun, Oct 6, 3:28 AM
F2905106: D8479.id28639.diff
Sun, Oct 6, 3:26 AM
F2904482: D8479.diff
Sun, Oct 6, 1:30 AM
Unknown Object (File)
Fri, Sep 27, 4:07 PM
Unknown Object (File)
Fri, Sep 27, 4:07 PM
Unknown Object (File)
Fri, Sep 27, 4:07 PM
Unknown Object (File)
Fri, Sep 27, 4:07 PM
Unknown Object (File)
Fri, Sep 27, 4:06 PM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
ginsu
Commits
rCOMM78707c7daef9: [web] Update the change role icon on web
Summary

The icon for the change role action item in the members modal is currently a plus-circle. After some discussion, we need to use the new edit-1 icon part of the CommIcon component.

MenuItem defaults to using a SWMansionIcon if a icon name is provided:

let menuItemIcon = iconComponent;
if (icon) {
  menuItemIcon = <SWMansionIcon size="100%" icon={icon} />;
}

so instead I used the existing iconComponent prop to pass in the CommIcon we're using.

Test Plan

The new icon looks as expected

Screenshot 2023-07-11 at 6.09.25 PM.png (328×956 px, 28 KB)

Diff Detail

Repository
rCOMM Comm
Branch
change_role_icon
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Jul 11 2023, 3:08 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 11 2023, 3:08 PM
rohan edited the test plan for this revision. (Show Details)Jul 11 2023, 3:09 PM
rohan attached a referenced file: F632135: Screenshot 2023-07-11 at 6.09.25 PM.png. (Show Details)
Harbormaster completed remote builds in B20884: Diff 28606.Jul 11 2023, 3:27 PM
rohan requested review of this revision.Jul 11 2023, 3:27 PM
ginsu requested changes to this revision.Jul 11 2023, 4:46 PM

Some concerns with the size prop

web/modals/threads/members/member.react.js
85

When dealing with the icon size we should use a numeric value, not a percentage

This revision now requires changes to proceed.Jul 11 2023, 4:46 PM
rohan added inline comments.Jul 11 2023, 5:07 PM
web/modals/threads/members/member.react.js
85

Oh sorry, I should have mentioned. I just replicated the way MenuItem renders the SWMansionIcon icon.

I could play around with a hard coded numeric value tomorrow to try to match the 'Remove User' icon with size 100%

rohan updated this revision to Diff 28638.EditedJul 12 2023, 7:58 AM

size="100%" --> size={18} after checking the icon rendered by MenuItem with size=100%'s svg dimensions

Screenshot 2023-07-12 at 10.57.07 AM.png (1×3 px, 626 KB)

rohan attached a referenced file: F633250: Screenshot 2023-07-12 at 10.57.07 AM.png. (Show Details)Jul 12 2023, 7:59 AM
Harbormaster completed remote builds in B20907: Diff 28638.Jul 12 2023, 8:15 AM
ashoat added inline comments.Jul 12 2023, 9:43 AM
web/modals/threads/members/member.react.js
85 ↗(On Diff #28638)

We should define <CommIcon size={18} icon="user-edit" /> in the global scope

rohan updated this revision to Diff 28639.Jul 12 2023, 10:38 AM

Define commIconComponent in the global scope

Harbormaster completed remote builds in B20908: Diff 28639.Jul 12 2023, 10:55 AM
ginsu accepted this revision.Jul 12 2023, 7:52 PM

thanks for fixing this!

This revision is now accepted and ready to land.Jul 12 2023, 7:52 PM
Closed by commit rCOMM78707c7daef9: [web] Update the change role icon on web (authored by rohan). · Explain WhyJul 14 2023, 1:49 PM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM78707c7daef9: [web] Update the change role icon on web.

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
members/
3 lines

Diff 28606

View Options

web/modals/threads/members/member.react.js

Loading...