Paths

Table of Contentst

Differential D15347

[web] choosing protocol type while creating a thread
ClosedPublic
Actions

Authored by kamil on Sep 15 2025, 10:12 AM.

Tags

None

Referenced Files

	F13582989: Screen Recording 2025-09-15 at 11.38.37.mov
	Sep 15 2025, 10:13 AM

Subscribers

Details

Reviewers

tomek
bartek

Commits

rCOMMe2136a3450ec: [web] choosing protocol type while creating a thread

Summary

Depends on D15346

Test Plan

Tested entire stack end-to-end

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Sep 15 2025, 10:12 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptSep 15 2025, 10:12 AM

kamil edited the summary of this revision. (Show Details)Sep 15 2025, 10:13 AM

Harbormaster failed remote builds in B35726: Diff 50303!Sep 15 2025, 10:18 AM

kamil published this revision for review.Sep 15 2025, 10:20 AM

kamil added inline comments.

web/assets/comm-logo.react.js
5 ↗	(On Diff #50303)	I was trying to use `comm-icon-config.json` too but I failed to make it work
web/components/protocol-icon.react.js
50 ↗	(On Diff #50303)	not sure if I am breaking some convention with using styles inline, but I saw this a lot in the codebase

kamil added a child revision: D15348: [native] choosing protocol type while creating a thread.Sep 15 2025, 10:21 AM

tomek accepted this revision.Sep 15 2025, 1:48 PM

tomek added inline comments.

web/components/protocol-icon.react.js
50 ↗	(On Diff #50303)	It's controversial. It's safer to wrap it with `useMemo`
web/components/select-protocol-dropdown.react.js
29 ↗	(On Diff #50303)	Isn't it safer to set `false`?
39 ↗	(On Diff #50303)	We should create a class in css file and use it here.
108–119 ↗	(On Diff #50303)	Let's use css classes for these styles

This revision is now accepted and ready to land.Sep 15 2025, 1:48 PM

ashoat added inline comments.Sep 15 2025, 5:43 PM

web/components/protocol-icon.react.js
50 ↗	(On Diff #50303)	In this case it doesn't matter since we are getting another prop (`iconComponent`) that isn't memoized

address review

web/components/select-protocol-dropdown.react.js
29 ↗	(On Diff #50303)	It won't work then, because you might want to click multiple times on the dropdown to hide and show it. I think you meant setting it to false when selecting protoco, but this is handled in `onOptionSelection`

Harbormaster failed remote builds in B35749: Diff 50334!Sep 16 2025, 10:14 AM

This revision was landed with ongoing or failed builds.Sep 16 2025, 10:49 AM

Closed by commit rCOMMe2136a3450ec: [web] choosing protocol type while creating a thread. · Explain Why

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMMe2136a3450ec: [web] choosing protocol type while creating a thread.

Revision Contents
Changeset List

Path

Size

lib/

shared/

comm-icon-config.json

25 lines

web/

assets/

comm-logo.react.js

31 lines

chat/

chat-thread-composer.css

22 lines

chat-thread-composer.react.js

30 lines

thread-top-bar.react.js

50 lines

comm-icon.react.js

3 lines

components/

protocol-icon.react.js

67 lines

select-protocol-dropdown.css

100 lines

select-protocol-dropdown.react.js

123 lines

Diff 50337

lib/shared/comm-icon-config.json

Loading...

web/assets/comm-logo.react.js

Loading...

web/chat/chat-thread-composer.css

Loading...

web/chat/chat-thread-composer.react.js

Loading...

web/chat/thread-top-bar.react.js

Loading...

web/comm-icon.react.js

Loading...

web/components/protocol-icon.react.js

Loading...

web/components/select-protocol-dropdown.css

Loading...

web/components/select-protocol-dropdown.react.js

Loading...