Page MenuHomePhabricator
Differential D5119

[Chore] Cleanup tunnelbroker CMakeLists.txt
ClosedPublic
Actions

Authored by • jon on Sep 12 2022, 1:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 7:26 PM
Unknown Object (File)
Fri, Mar 21, 1:07 AM
Unknown Object (File)
Thu, Mar 20, 9:44 PM
Unknown Object (File)
Tue, Mar 18, 8:40 AM
Unknown Object (File)
Tue, Mar 18, 1:45 AM
Unknown Object (File)
Tue, Mar 18, 1:45 AM
Unknown Object (File)
Mon, Mar 17, 4:11 AM
Unknown Object (File)
Mon, Mar 17, 4:11 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
varun
• abosh
• max
• karol
Commits
rCOMM237416184fb3: [Chore] Cleanup tunnelbroker CMakeLists.txt
Summary

Noticed some dead code from D4671

Test Plan

tunnelbroker CI passes

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jon created this revision.Sep 12 2022, 1:50 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptSep 12 2022, 1:50 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 12 2022, 1:51 PM
Harbormaster failed remote builds in B12094: Diff 16601!
• jon added a parent revision: D5118: [Services] Use BUILD_TESTING for tunnelbroker build.Sep 12 2022, 2:03 PM
• jon updated this revision to Diff 16608.Sep 12 2022, 2:06 PM

Rebase on top of docker fixes

• jon updated this revision to Diff 16611.Sep 12 2022, 2:09 PM

Rebase

Harbormaster completed remote builds in B12101: Diff 16608.Sep 12 2022, 2:16 PM
Harbormaster completed remote builds in B12104: Diff 16611.Sep 12 2022, 2:19 PM
• jon requested review of this revision.Sep 12 2022, 2:19 PM
atul accepted this revision.Sep 13 2022, 9:34 AM

In addition to tunnelbroker CI, was able to run docker-compose build --no-cache tunnelbroker-server successfully on Apple silicon

This revision is now accepted and ready to land.Sep 13 2022, 9:34 AM
Closed by commit rCOMM237416184fb3: [Chore] Cleanup tunnelbroker CMakeLists.txt (authored by • jon). · Explain WhySep 13 2022, 11:45 AM
This revision was automatically updated to reflect the committed changes.
• jon added a commit: rCOMM237416184fb3: [Chore] Cleanup tunnelbroker CMakeLists.txt.
ashoat added inline comments.Sep 15 2022, 7:04 PM
services/tunnelbroker/CMakeLists.txt
77–81 ↗(On Diff #16632)

I'm a bit confused, what's the point of this line now?

• jon added inline comments.Sep 15 2022, 7:06 PM
services/tunnelbroker/CMakeLists.txt
77–81 ↗(On Diff #16632)

Good point, guess this isn't needed either.

ashoat added inline comments.Sep 16 2022, 8:03 AM
services/tunnelbroker/CMakeLists.txt
77–81 ↗(On Diff #16632)

D5160

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
5 lines

Diff 16611

View Options

services/tunnelbroker/CMakeLists.txt

  • This file was added.
div.containerViewerLiked {
padding: 4px;
border-radius: 4px;
background-color: var(--shades-black-80);
}
div.container:hover {
cursor: pointer;
}
tomekUnsubmitted
Not Done
Inline Actions

For other tooltip actions we don't need to manually set this style. Can we use the mechanism we already have, or are there any reasons for not doing so?

tomek: For other tooltip actions we don't need to manually set this style. Can we use the mechanism we…
ginsuAuthorUnsubmitted
Done
Inline Actions

I believe the other icons don't need to set this style because it is built into IcomoonReact. When I switched the Like action tooltip button with one of the icons the hover style just works out of the box. Wondering if @atul has more context since he is pretty familiar with the icons stuff

ginsu: I believe the other icons don't need to set this style because it is built into `IcomoonReact`.
tomekUnsubmitted
Not Done
Inline Actions

It wouldn't make too much sense for Icomoon to provide hover effects for icons because they could be used in other places - not only as buttons. It seems like the style on hover is applied in message-tooltip.css

div.messageActionButtons svg:hover {
  cursor: pointer;
  color: var(--fg);
}

so it should be enough to apply this style also to the new button.

tomek: It wouldn't make too much sense for `Icomoon` to provide hover effects for icons because they…
ginsuAuthorUnsubmitted
Done
Inline Actions

Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes

ginsu: Ahhhhh that makes sense, thank you for clarifying, I will make the necessary changes