Page MenuHomePhabricator
Differential D4679

[web] Introduce `ThreadSettingsRelationshipButton` component
ClosedPublic
Actions

Authored by • jacek on Jul 29 2022, 3:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 3, 9:15 PM
Unknown Object (File)
Mon, Mar 3, 9:15 PM
Unknown Object (File)
Mon, Mar 3, 9:15 PM
Unknown Object (File)
Mon, Mar 3, 9:15 PM
Unknown Object (File)
Mon, Mar 3, 9:14 PM
Unknown Object (File)
Mon, Mar 3, 8:54 PM
Unknown Object (File)
Jan 27 2025, 6:54 AM
Unknown Object (File)
Jan 26 2025, 3:53 PM
View All Files
Subscribers
• abosh
• adrian
ashoat

Details

Reviewers
tomek
atul
ashoat
Commits
rCOMM1ac8fb7357f8: [web] Introduce `ThreadSettingsRelationshipButton` component
Summary

Create a button component that dispatches a matching relationship action on click. It will be displayed in "Relationship" tab in thread settings modal on web

Screenshot_Google Chrome_2022-07-29_122412.png (665×460 px, 26 KB)

Test Plan

The component is not used yet. Confirm it works after introducing relationship tab in thread settings modal in next diffs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• jacek created this revision.Jul 29 2022, 3:04 AM
Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptJul 29 2022, 3:04 AM
Harbormaster completed remote builds in B10968: Diff 15090.Jul 29 2022, 3:16 AM
• jacek requested review of this revision.Jul 29 2022, 3:16 AM
• jacek added a child revision: D4680: [web] Introduce `ThreadSettingsRelationshipTab` component.Jul 29 2022, 3:24 AM
• jacek edited the summary of this revision. (Show Details)
• jacek added parent revisions: D4678: [lib] Introduce shared `getRelationshipActionText` function, D4677: [lib] Introduce shared `getRelationshipDispatchAction` function.Jul 29 2022, 3:27 AM
tomek requested changes to this revision.Jul 29 2022, 5:44 AM
tomek added a reviewer: ashoat.

I think we should consider having consistent relationship action colors through the app. So green when we send / accept and red when we decline / block. @ashoat what do you think?

web/modals/threads/settings/thread-settings-relationship-button.react.js
34–42 ↗(On Diff #15090)

It looks like we should merge these together and have e.g. getRelationshipActionSpec which returns { action, text }. This is especially important because we probably should introduce yet another prop which is button color.

46–67 ↗(On Diff #15090)

It doesn't seem necessary for updateRelationshipsAction to take an argument when the action is always equal to relationshipAction.

Another thing is that the second parameter of dispatchActionPromise is a promise, so it definitely shouldn't be named updateRelationshipsAction.

63 ↗(On Diff #15090)

We probably don't need to use dispatchActionPromise if we ignore request state. But we shouldn't ignore it, because now it is possible to click the buttons multiple times. We should render the loading state if a request is in progress.

This revision now requires changes to proceed.Jul 29 2022, 5:44 AM
• jacek added a comment.Jul 29 2022, 6:27 AM
In D4679#134441, @tomek wrote:

I think we should consider having consistent relationship action colors through the app. So green when we send / accept and red when we decline / block. @ashoat what do you think?

In native, we use the same color for every action. Probably such change will require adding some "green" variant of button because we don't have such one on web.

web/modals/threads/settings/thread-settings-relationship-button.react.js
46–67 ↗(On Diff #15090)

Both these things are taken directly from native code, so probably corresponding changes should also be implemented there.

63 ↗(On Diff #15090)

Hmm... I did it exactly as it's in native app. I checked this part of code, and it seems that in mobile we don't prevent pressing buttons multiple times and dispatching multiple actions while waiting for server response.
So probably the code on native should be changed too, what do you think?

• jacek added inline comments.Jul 29 2022, 8:22 AM
web/modals/threads/settings/thread-settings-relationship-button.react.js
46–67 ↗(On Diff #15090)

Another thing is that the second parameter of dispatchActionPromise is a promise, so it definitely shouldn't be named updateRelationshipsAction.

We have this naming convention in multiple places in our code, that the second parameter of dispatchActionPromise is execution of function which name ends with action.
Do you have a suggestion of how it should be named here?

• jacek updated this revision to Diff 15101.Jul 29 2022, 8:47 AM

improvements after Tomek's review

Harbormaster completed remote builds in B10973: Diff 15101.Jul 29 2022, 8:59 AM
ashoat accepted this revision.Jul 29 2022, 10:39 AM
ashoat added 1 blocking reviewer(s): tomek.
In D4679#134458, @jacek wrote:
In D4679#134441, @tomek wrote:

I think we should consider having consistent relationship action colors through the app. So green when we send / accept and red when we decline / block. @ashoat what do you think?

In native, we use the same color for every action. Probably such change will require adding some "green" variant of button because we don't have such one on web.

Maybe we can create a backlog task? I agree it would be nice to be more consistent with the colors, but don't want to slow down our progress on feature work. Ultimately we'll probably end up needing to redesign most of this after a designer takes a look.

tomek accepted this revision.Aug 1 2022, 6:37 AM
tomek added inline comments.
web/modals/threads/settings/thread-settings-relationship-button.react.js
34–42 ↗(On Diff #15090)

My idea was to go even one step further and create a function that returns an object - just to keep the related things next to each other.

46–67 ↗(On Diff #15090)

I don't think we have a convention for this - the naming seems mostly inconsistent. I have a couple of ideas for the name: we can have updateRelationshipsPromiseCreator, updateRelationshipsActionPromise, etc. Ending this name with action sounds misleading, because this is not an action.

On the other hand, keeping the current name makes some sense, because it is probably the most common naming in our project, but still, I don't think this is good name.

63 ↗(On Diff #15090)

I think we can start by creating a task to block the buttons on web and native. Not sure how our current logic works: if we don't update the UI optimistically and instead wait for the server to send an updated relationship status, it should be fine to keep the current implementation. But if we do optimistic updates, the following scenario could happen:

  1. User clicks a button to block another user
  2. UI is updated and now a user can see a button to e.g. send friend request
  3. User sends a friend request
  4. Server receives the requests but processes the second one faster
  5. The final state is that the user is blocked

Which isn't a really big issue, and should be a corner case, but we should be able to easily avoid it.

An example of the implementation can be found in add-users-list.react.js.

This revision is now accepted and ready to land.Aug 1 2022, 6:37 AM
• jacek updated this revision to Diff 15203.Aug 2 2022, 3:05 AM

Moved loading status selector, change updateRelationshipsAction callback name

• jacek added inline comments.Aug 2 2022, 3:15 AM
web/modals/threads/settings/thread-settings-relationship-button.react.js
34–42 ↗(On Diff #15090)

Discussed it offline. I'll create backlog task for this

Harbormaster completed remote builds in B11044: Diff 15203.Aug 2 2022, 3:17 AM
• jacek added a comment.Aug 2 2022, 3:25 AM
In D4679#134595, @ashoat wrote:
In D4679#134458, @jacek wrote:
In D4679#134441, @tomek wrote:

I think we should consider having consistent relationship action colors through the app. So green when we send / accept and red when we decline / block. @ashoat what do you think?

In native, we use the same color for every action. Probably such change will require adding some "green" variant of button because we don't have such one on web.

Maybe we can create a backlog task? I agree it would be nice to be more consistent with the colors, but don't want to slow down our progress on feature work. Ultimately we'll probably end up needing to redesign most of this after a designer takes a look.

Cerated task: https://linear.app/comm/issue/ENG-1532/introduce-colors-for-relationship-actions

Closed by commit rCOMM1ac8fb7357f8: [web] Introduce `ThreadSettingsRelationshipButton` component (authored by • jacek). · Explain WhyAug 3 2022, 4:27 AM
This revision was automatically updated to reflect the committed changes.
• jacek added a commit: rCOMM1ac8fb7357f8: [web] Introduce `ThreadSettingsRelationshipButton` component.

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
settings/
79 lines

Diff 15254

View Options

web/modals/threads/settings/thread-settings-relationship-button.react.js

#pragma once #pragma once
#include "AwsTools.h" #include "AwsTools.h"
#include "Constants.h" #include "Constants.h"
#include "DatabaseEntitiesTools.h" #include "DatabaseEntitiesTools.h"
#include "Tools.h" #include "Tools.h"
#include <aws/core/Aws.h> #include <aws/core/Aws.h>
#include <aws/core/utils/Outcome.h> #include <aws/core/utils/Outcome.h>
#include <aws/dynamodb/model/AttributeDefinition.h> #include <aws/dynamodb/model/AttributeDefinition.h>
#include <aws/dynamodb/model/DeleteItemRequest.h> #include <aws/dynamodb/model/DeleteItemRequest.h>
#include <aws/dynamodb/model/DescribeTableRequest.h> #include <aws/dynamodb/model/DescribeTableRequest.h>
#include <aws/dynamodb/model/DescribeTableResult.h> #include <aws/dynamodb/model/DescribeTableResult.h>
#include <aws/dynamodb/model/GetItemRequest.h> #include <aws/dynamodb/model/GetItemRequest.h>
#include <aws/dynamodb/model/PutItemRequest.h> #include <aws/dynamodb/model/PutItemRequest.h>
#include <aws/dynamodb/model/QueryRequest.h>
#include <memory> #include <memory>
#include <string> #include <string>
namespace comm { namespace comm {
namespace network { namespace network {
namespace database { namespace database {
Show All 22 Linespublic:
void removeSessionSignItem(const std::string &deviceID); void removeSessionSignItem(const std::string &deviceID);
void putPublicKeyItem(const PublicKeyItem &item); void putPublicKeyItem(const PublicKeyItem &item);
std::shared_ptr<PublicKeyItem> findPublicKeyItem(const std::string &deviceID); std::shared_ptr<PublicKeyItem> findPublicKeyItem(const std::string &deviceID);
void removePublicKeyItem(const std::string &deviceID); void removePublicKeyItem(const std::string &deviceID);
void putMessageItem(const MessageItem &item); void putMessageItem(const MessageItem &item);
std::shared_ptr<MessageItem> findMessageItem(const std::string &messageID); std::shared_ptr<MessageItem> findMessageItem(const std::string &messageID);
std::vector<std::shared_ptr<MessageItem>>
findMessageItemsByReceiver(const std::string &toDeviceID);
void removeMessageItem(const std::string &messageID); void removeMessageItem(const std::string &messageID);
}; };
} // namespace database } // namespace database
} // namespace network } // namespace network
} // namespace comm } // namespace comm