Paths

Table of Contentst

[web] [fix] add icons with default paddings
ClosedPublic
Actions

Authored by • benschac on Feb 28 2022, 9:41 AM.

Details

Reviewers

atul
tomek
ashoat

Commits

rCOMM336f32113f4d: [web] [fix] add icons with default paddings

Summary

Re-import icons with default paddings from icon pack.

Imported Icons

Image 2022-02-28 at 12.43.02 PM.jpg (1×3 px, 395 KB)

The diff is a little bigger because I had to delete all of the old icons and re-import but they got out of order. If it's a big deal I can go back and order the icons to their previous state.

Note: All of the icons will appear smaller now that paddings are added back to the fonts. Looking at the UI and figma they're now (almost) the correct size. The only exception that I found so far of icons that need to be updated in a follow-up diff are in the input (picture and send icons) which are linked in this stack. Additional icon size adjustments are addressed in following diffs in this stack.

Test Plan

inspect icons, they should have padding

Image 2022-02-28 at 12.38.50 PM.jpg (242×356 px, 10 KB)

This diff doesn't update layout or fix any sizing issues with imported icons. All it's doing is replacing the old icons without paddings with the icons that do have paddings.

I grepped the repo and updated every icon in the following diffs to address the updated icons with paddings.

Just confirming here all of the files that have <SWMansionIcon /> component. They have either been updated or currently match the design.

calendar.js: N/A there wasn't design for this page. No updates.
chat-input-bar: plus, send and image icons have been updated both in size and layout.
chat-thread-ancestors: cloud, and chevron-right icons have been updated to correct sizes
chat-thread-list-item-menu: menu-vertical is the correct size per figma designs.
chat-thread-list-item: is the correct size
chat-thread-tab: icons are updated to 24px, the correct size.
message-action-buttons: is correct size 18px.
message-reply-button: is correct size 18px.
sidebar-item: right angle arrow isn't a SWM icon (the long sidebar arrow), doesn't need to be updated.
thread-menu: menu icon has been updated to the correct size and button element padding has been removed.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• benschac created this revision.Feb 28 2022, 9:41 AM

Herald added subscribers: • karol, • adrian, ashoat. · View Herald TranscriptFeb 28 2022, 9:41 AM

• benschac edited the summary of this revision. (Show Details)Feb 28 2022, 9:44 AM

• benschac edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B7064: Diff 9954.Feb 28 2022, 9:45 AM

• benschac requested review of this revision.Feb 28 2022, 9:45 AM

@benschac, doesn't this lead to visual regressions because the paddings are changed? Should there be another diff in this stack that resets CSS to match? If so, please mention that sort of thing in the diff description! (Sorry if I missed it)

This revision now requires changes to proceed.Feb 28 2022, 6:13 PM

Seeing now that D3300 is part of the same stack, but you didn't set a "Depends On" relationship. Please always set the relationship between diffs you create

• benschac added a child revision: D3300: [web] fix input icons to be the correct size.Feb 28 2022, 6:35 PM

ashoat mentioned this in D3300: [web] fix input icons to be the correct size.Feb 28 2022, 7:49 PM

• benschac mentioned this in D3267: [web] [fix] sidebar app switcher correct width.Mar 1 2022, 11:23 AM

• benschac edited the summary of this revision. (Show Details)Mar 1 2022, 1:46 PM

In D3299#88869, @ashoat wrote:

@benschac, doesn't this lead to visual regressions because the paddings are changed? Should there be another diff in this stack that resets CSS to match? If so, please mention that sort of thing in the diff description! (Sorry if I missed it)

It's lead to one visual regression that I have seen so far. I'm taking another look now and double-checking if anything looks off. I'll address any regressions in following diffs in this stack.

• benschac requested review of this revision.Mar 2 2022, 7:57 AM

Are these icons duplicates of each other? Or is one intentionally smaller than the other?

Is there a reason we aren't importing the entire icon pack in one go?

(also noticed the "Filled" icon is still present, wouldn't this be the right time to fix the name of that icon as well?)

This revision now requires changes to proceed.Mar 2 2022, 8:01 AM

• benschac added a child revision: D3294: [web] [fix] updated icons component.Mar 2 2022, 8:59 AM

• benschac removed a child revision: D3294: [web] [fix] updated icons component.

• benschac added a child revision: D3294: [web] [fix] updated icons component.

In D3299#89338, @atul wrote:

Are these icons duplicates of each other? Or is one intentionally smaller than the other?

Yes, one is intentionally smaller. I remember seeing them as different icons in figma.

Is there a reason we aren't importing the entire icon pack in one go?

Ideally, to not add icons we don't need. I don't think there are many more icons. I can look though the entire front-end and pull them out: https://linear.app/comm/issue/ENG-822/import-all-icons-from-swm-icon-pack

(also noticed the "Filled" icon is still present, wouldn't this be the right time to fix the name of that icon as well?)

I don't think it is. The name of the icon is ''message-filled-round' per: https://phabricator.ashoat.com/D3294. If it wasn't changed from filled the icon wouldn't load.

• benschac requested review of this revision.Mar 2 2022, 9:18 AM

• benschac added a child revision: D3325: [web] fix icons sizes in community picker side bar.Mar 2 2022, 10:18 AM

• benschac added a child revision: D3326: [web] [fix] update cloud icon to correct size with paddings.Mar 2 2022, 10:35 AM

It's lead to one visual regression that I have seen so far. I'm taking another look now and double-checking if anything looks off. I'll address any regressions in following diffs in this stack.

Please make sure your analysis to determine visual regressions is not based on a loose, general visual inspection, but rather is based on searching through the codebase (eg. git grep) for any and all codepoints that are affected by this change, and then doing a visual inspection for each individual location.

This will take more time but it is critical to avoiding regressions. You should always take great care to make sure your changes never introduce regressions.

Back to you – please update the test plan to reflect a thorough inspection of every single location in code that is affected by this change

This revision now requires changes to proceed.Mar 2 2022, 10:57 PM

• benschac added a child revision: D3329: [web] [fix] update join button icon with padding, adjust layout.Mar 3 2022, 5:30 AM

• benschac added a child revision: D3330: [web] [fix] chevron size + plus spacing.Mar 3 2022, 5:58 AM

• benschac edited the test plan for this revision. (Show Details)Mar 3 2022, 8:50 AM

• benschac edited the test plan for this revision. (Show Details)

• benschac added a child revision: D3331: [web] [fix] update menu button size, correct addition bottom padding.

In D3299#89575, @ashoat wrote:

Back to you – please update the test plan to reflect a thorough inspection of every single location in code that is affected by this change

I updated the test plan with every instance where the icon component is used. All changes are linked in the diff stack. Additionally, some of the icons were a bit off-center which I updated/corrected.

• benschac requested review of this revision.Mar 3 2022, 9:01 AM

atul accepted this revision.Mar 3 2022, 11:20 AM

Ideally, to not add icons we don't need. I don't think there are many more icons. I can look though the entire front-end and pull them out: https://linear.app/comm/issue/ENG-822/import-all-icons-from-swm-icon-pack

IMO I think we should just add the entire icon pack and call it good so we can avoid having to do all this extra work just to use an icon.

We could look into using https://github.com/kamilagraf/react-swm-icon-pack?

they're all basically here, I just need to take another pass. Additionally, we probably still want to use Icomoon for the custom icons, no?

Thanks for the detailed test plan!!

This revision is now accepted and ready to land.Mar 3 2022, 10:32 PM

• benschac added a child revision: D3346: [web] add style prop to icon component.Mar 4 2022, 11:33 AM

Closed by commit rCOMM336f32113f4d: [web] [fix] add icons with default paddings (authored by • benschac). · Explain WhyMar 9 2022, 10:38 AM

This revision was automatically updated to reflect the committed changes.

• benschac added a commit: rCOMM336f32113f4d: [web] [fix] add icons with default paddings.