In D7793#232559, @michal wrote:

These //TODO comments make it hard to review this stack because now the diffs depend on both previous and future diffs. This diff in particular seem like it would be easy to merge with D7824. Is there a reason why you did it this way?

As far as I know, diffs should be small, and do only one thing. The division between those diffs is simple: one diff adds the button itself to the tooltip, and the other adds its functionality (entering edit state).

Also, this diff cannot depend on previous diffs, because it is first in the stack.

If it is necessary I can merge those diffs, but in my opinion, the difference here is clear.

As far as I know, diffs should be small, and do only one thing. The division between those diffs is simple: one diff adds the button itself to the tooltip, and the other adds its functionality (entering edit state).
Also, this diff cannot depend on previous diffs, because it is first in the stack.

The main reason why diffs are supposed to be small is to be easier to review. Introducing the comments means that I need to search for the diff that introduces the missing component and for one that replaces the comment with logic to review thoroughly. This is what I meant by "dependency" (not a formal dependency between diffs on phabricator).

The other big thing is that diffs should stand on their own. If you landed only part of the stack (without the diff replacing the comment with logic), or we needed to revert some diffs and didn't revert this one, we would be left with a non-working tooltip button that is displayed but doesn't do anything.

Separately, at least on my laptop (on firefox) the button doesn't look quite right:

I can see that on figma that all buttons are outlines and there's a bar between reactions and the rest of the buttons. Not sure if it will be handled on some other occasion or if there's some more context on this, but I feel like they should be at least consistent.

It's true that diffs should be small, but that is just an "instrumental goal". The real goal is to make diffs readable and clear to reviewers, and usually small diffs are more readable and clear. But that's not always the case... often, splitting a diff into two can make it harder to review, especially if the two parts depend closely on each other. In this case I agree with @michal that it would be easier to review if D7824 and this diff were merged.

All other icons are filled in.

I can see that on figma that all buttons are outlines and there's a bar between reactions and the rest of the buttons. Not sure if it will be handled on some other occasion or if there's some more context on this, but I feel like they should be at least consistent.

I think we should get a filled-in icon for this – thanks for checking this out and finding this issue. @ted, can you take a look? cc @ginsu

In D7793#232996, @michal wrote:

Accepting, but please fix the icon before landing!

Added it to the landing checklist: https://linear.app/comm/issue/ENG-3906/change-edit-icon-on-web-to-filled-one