Page MenuHomePhabricator

[web] Connect `ComposeSubchannel` modal with keyserver
ClosedPublic

Authored by jakub on Sep 22 2022, 5:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 11, 12:02 PM
Unknown Object (File)
Feb 20 2024, 9:42 PM
Unknown Object (File)
Feb 19 2024, 4:48 PM
Unknown Object (File)
Feb 19 2024, 4:03 PM
Unknown Object (File)
Feb 19 2024, 2:29 PM
Unknown Object (File)
Feb 19 2024, 2:03 PM
Unknown Object (File)
Feb 19 2024, 12:18 PM
Unknown Object (File)
Feb 19 2024, 11:32 AM

Details

Summary

Depends on D5187
Context: here

Connecting ComposeSubchannelModal with keyserver api. Now, we can create subchannels on web.

Test Plan
  1. Go to any chat
  2. Open thread menu
  3. Open Create new subchannel
  4. Try to create any subchannel via ComposeSubchannel

For now, we don't have implemented announcement channels yet. In this case, when we try to create any type of announcement channels, this communiqué will appear:

image.png (172×762 px, 24 KB)

In other cases, after successfully thread creation, we will be redirected to a newly created subchannel.

Diff Detail

Repository
rCOMM Comm
Branch
jakub/channel-composer
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jakub edited the test plan for this revision. (Show Details)
jakub added reviewers: tomek, atul.

Setting @tomek as blocking on this one since he has more context

web/modals/threads/create/compose-subchannel-modal.react.js
84 ↗(On Diff #16981)

What's the + here for?

100 ↗(On Diff #16981)

minor typo: should be "announcement"

tomek requested changes to this revision.Sep 27 2022, 2:58 AM
tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.react.js
27 ↗(On Diff #16981)

matrix doesn't explain the purpose of this constant. Maybe threadTypesPerVisibility would be more descriptive?

73 ↗(On Diff #16981)

I don't think it is necessary to create a state for loading. You should use createLoadingStatusSelector instead.

97 ↗(On Diff #16981)

This is strange that we're setting loading to false only when there's an error. How the loading indicator behaves when a thread is created successfully?

This revision now requires changes to proceed.Sep 27 2022, 2:58 AM
jakub marked 3 inline comments as done.

Adjust to code review:

  • fix typo
  • change matrix name to threadTypesPerVisibility
  • use createLoadingStatusIndicator instead of using component state
web/modals/threads/create/compose-subchannel-modal.react.js
84 ↗(On Diff #16981)

It's an alternative for parseInt function. By default, announcement is boolean value thus when we try to use it as index in threadTypesMatrix in JavaScript, we need to convert it to int.

Here is sample how it works:

image.png (384×564 px, 48 KB)

97 ↗(On Diff #16981)

After successfully thread creation, modal disappear, so we don't need to return loading state to false.
In the latest revision, we use createLoadingStatusIndicator instead using local component state, so we don't need to use this state anymore.

Hey @jakub, your attachment didn't work... an unfortunate quirk of Phabricator. Details on how to do it here

tomek requested changes to this revision.Sep 30 2022, 4:51 AM
tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.react.js
34 ↗(On Diff #17198)

Maybe secret?

84 ↗(On Diff #16981)

This is really fragile. JS conversions are terrible and relying on them is extremely risky. Also, it isn't obvious that the position in threadTypesPerVisibility matters and it's easy to break this. I think it would be better to have an object instead of an array and use announcement value in ternary expression. But still, this complication isn't necessary and I think the best solution here is to have a simple function that takes visibilityType and announcement and returns threadType as a result. (just like pendingThreadType in thread-utils does).

97 ↗(On Diff #16981)

I don't see createLoadingStatusIndicator being used here. Is it in a different diff or this diff will be updated?

This revision now requires changes to proceed.Sep 30 2022, 4:51 AM

Adjust to code review:

  • change closed to secret
  • replace threadTypesPerVisibility with getThreadType function
web/modals/threads/create/compose-subchannel-modal.react.js
97 ↗(On Diff #16981)

Yes, I used it in the latest diff update

tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.react.js
107 ↗(On Diff #17239)

I think there are some other possible causes for this error and we shouldn't assume that this is due to announcement not being supported. So maybe e.message === 'invalid_parameters' && announcement might be a better condition?

222 ↗(On Diff #17239)

Should the button be disabled also when loadingState === 'loading'?

This revision is now accepted and ready to land.Sep 30 2022, 7:06 AM

Adjust to code review

web/modals/threads/create/compose-subchannel-modal.react.js
222 ↗(On Diff #17239)

In stepper ActionButton setting`loading` to true also disabling it. (code)