fixes [6654] Clicking on user avatar (profile picture) in header doesn't open conversation details #6658

bhaskarraksahb · 2023-11-05T16:40:56Z

First time contributor checklist:

[X ] I have read the README and Contributor Guidelines
[X ] I have signed the Contributor Licence Agreement

Contributor checklist:

[X ] My contribution is not related to translations.
[X ] My commits are in nice logical chunks with good commit messages
[X ] My changes are rebased on the latest main branch
[X ] A yarn ready run passes successfully (more about tests here)
[X ] My changes are ready to be shipped to users

Description

fixes 6654
Updated button tag to be before div tag so that avatar comes inside button.

change.mp4

Co-authored-by: Fedor Indutny <[email protected]>

Co-authored-by: ayumi-signal <[email protected]>

Co-authored-by: Fedor Indutny <[email protected]>

jamiebuilds-signal · 2023-11-06T16:18:40Z

Brought this up with our designers. This would have to be conditionally applied when the conversation has a story to view. In that case the avatar is rendered with a ring around it and is clickable to view the user's story.

jamiebuilds-signal · 2023-11-06T16:24:02Z

Okay, yes we want to make this change, but could you make it work in the case of a story?

Co-authored-by: Jamie Kyle <[email protected]>

Co-authored-by: Rashad Sookram <[email protected]>

Co-authored-by: ayumi-signal <[email protected]>

ayumi-signal · 2023-11-06T22:27:34Z

@bhaskarraksahb Thanks for submitting this pr!

As a note the change also seems to alter the margins on the top avatar. It would be nice if the margins can be preserved, so the header's avatar can line up on the left with conversation avatars (group chat) or conversation text bubbles (direct chat). See screenshots below.

Before:

After this pr:

Co-authored-by: Fedor Indutny <[email protected]>

bhaskarraksahb · 2023-11-07T16:17:48Z

@ayumi-signal @jamiebuilds-signal sure

Co-authored-by: ayumi-signal <[email protected]>

Co-authored-by: Fedor Indutny <[email protected]>

Co-authored-by: ayumi-signal <[email protected]>

Co-authored-by: Fedor Indutny <[email protected]>

Co-authored-by: ayumi-signal <[email protected]>

bhaskarraksahb · 2023-11-08T17:23:05Z

@ayumi-signal have updated the component please take a look thanks!

ayumi-signal

Thanks so much for making changes! It's looking better, and I also confirmed visually and functionally that it's working correctly. With a little refactor I think it can be merged. Please check the review comments.

ayumi-signal · 2023-11-08T18:32:33Z

ts/components/conversation/ConversationHeader.tsx

@@ -242,7 +242,7 @@ export class ConversationHeader extends React.Component<PropsType, StateType> {
                    storyViewMode: StoryViewModeType.User,
                  });
                }
-              : undefined
+              : this.getActionForType()


It looks like this function would be called a duplicate time from renderHeader().
Also renderAvatar() is only called from renderHeader().

What if we change renderAvatar() to take the onClick handler generated earlier?
It can look like this to indicate it's a fallback handler in case the stories handler isn't needed:

private renderAvatar(onClickFallback: undefined | (() => void)): ReactNode {

Then it will be simpler and we would not recreate the handler function.

ayumi-signal · 2023-11-08T18:34:10Z

ts/components/conversation/ConversationHeader.tsx

@@ -679,7 +679,7 @@ export class ConversationHeader extends React.Component<PropsType, StateType> {
    );
  }

-  private renderHeader(): ReactNode {
+  private getActionForType() {


Combined with the change to renderAvatar(), we would not need to recreate the onClick handler.
In that case it's ok to revert this change and keep onClick generation as part of renderHeader().

(As a side note, if hypothetically we wanted to keep this new onClick generator, it's maybe more clear to name it verbosely such as getHeaderOnClickHandler() because "getActionForType" can be confusing in a complex component.)

ayumi-signal · 2023-11-08T18:34:57Z

ts/components/conversation/ConversationHeader.tsx

-
+    return onClick;
+  }
+  private renderHeader(): ReactNode {


Same as above, I think we can revert this change and keep onClick generation as part of renderHeader(). Then we can pass the onClick to renderAvatar().

ayumi-signal · 2023-11-08T18:41:52Z

ts/components/conversation/ConversationHeader.tsx

@@ -905,4 +908,4 @@ function OutgoingCallButtons({
    default:
      throw missingCaseError(outgoingCallButtonStyle);
  }
-}
+}


The app's eslint style guide specifies trailing newlines. Can you revert this newline deletion?

Co-authored-by: trevor-signal <[email protected]>

bhaskarraksahb · 2023-11-10T16:33:34Z

Hi @ayumi-signal my bad, i should have thought of refactoring it a little bit before. Thanks Have updated the components please take a look.

ayumi-signal · 2023-11-10T23:42:48Z

Thanks so much for your work on this! It looks great now.
We've merged it internally, and it will be released with our next beta family 6.40.x.

For now, we can leave this PR open. Later it will automatically be marked as merged when we push our internal merge commits to a public branch.

ayumi-signal · 2023-11-15T23:57:51Z

This has been merged now. Nice work!
e13e436

automated-signal and others added 5 commits November 2, 2023 14:34

Strict types in RemoteConfig

ec5705c

Co-authored-by: Fedor Indutny <[email protected]>

Call Controls: Enhance width for viewport sizes

1bb1e3a

Co-authored-by: ayumi-signal <[email protected]>

Reduce flake and extend edit-test to include more send state updates

361468c

Username and username link integrity check

650c2f5

Co-authored-by: Fedor Indutny <[email protected]>

Updated div to be inside button

29ffac9

bhaskarraksahb changed the title ~~Updated div to be inside button~~ Clicking on user avatar (profile picture) in header doesn't open conversation details Nov 5, 2023

bhaskarraksahb changed the title ~~Clicking on user avatar (profile picture) in header doesn't open conversation details~~ fixes [6654] Clicking on user avatar (profile picture) in header doesn't open conversation details Nov 5, 2023

jamiebuilds-signal self-requested a review November 6, 2023 16:17

jamiebuilds-signal self-assigned this Nov 6, 2023

automated-signal and others added 3 commits November 6, 2023 08:40

Fix untranslated labels/tooltips in NavTabs

795293e

Co-authored-by: Jamie Kyle <[email protected]>

Add remote config for sending higher res screen share

62ac48f

Co-authored-by: Rashad Sookram <[email protected]>

Use Message received_at_ms to hide typing bubble

3f30a7b

Co-authored-by: ayumi-signal <[email protected]>

Better check for empty storage/master keys

d22828d

Co-authored-by: Fedor Indutny <[email protected]>

jamiebuilds-signal requested review from ayumi-signal and removed request for jamiebuilds-signal November 7, 2023 17:47

jamiebuilds-signal assigned ayumi-signal and unassigned jamiebuilds-signal Nov 7, 2023

automated-signal and others added 7 commits November 7, 2023 12:36

Fix call settings and participant page buttons

0de7f40

Co-authored-by: ayumi-signal <[email protected]>

Remove deprecated ContactDetails fields

6842129

Co-authored-by: Fedor Indutny <[email protected]>

Fix infinite stacking All Media with shortcut

15ddae1

Co-authored-by: ayumi-signal <[email protected]>

Fix UI bug with self-add by username in groups

7106352

Co-authored-by: Fedor Indutny <[email protected]>

Fix left pane banner colors

842332a

Co-authored-by: Fedor Indutny <[email protected]>

Show error when editing messages after max edits

308270f

Co-authored-by: ayumi-signal <[email protected]>

avatar to call pushconversation instead of undef

8c2ce6b

ayumi-signal reviewed Nov 8, 2023

View reviewed changes

automated-signal and others added 6 commits November 8, 2023 14:25

Fix group call video alignment in RTL

20356ac

Co-authored-by: trevor-signal <[email protected]>

Backport Language Picker

e6de7d8

Update WhatsNew

16186d5

Update strings

db78a63

v6.39.0-beta.1

824d252

Updated renderAvatar to accept callback

7e02619

Merge branch 'signalapp:main' into clickingAvatar

70c7d2a

ayumi-signal added PR: Ready to Merge PR: Merged and removed PR: Ready to Merge labels Nov 10, 2023

indutny-signal force-pushed the main branch from 824d252 to ab187ab Compare November 15, 2023 23:45

ayumi-signal closed this Nov 15, 2023

ayumi-signal mentioned this pull request Mar 7, 2024

Clicking on user avatar (profile picture) in header doesn't open conversation details #6654

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes [6654] Clicking on user avatar (profile picture) in header doesn't open conversation details #6658

fixes [6654] Clicking on user avatar (profile picture) in header doesn't open conversation details #6658

bhaskarraksahb commented Nov 5, 2023 •

edited

Loading

jamiebuilds-signal commented Nov 6, 2023

jamiebuilds-signal commented Nov 6, 2023

ayumi-signal commented Nov 6, 2023 •

edited

Loading

bhaskarraksahb commented Nov 7, 2023

bhaskarraksahb commented Nov 8, 2023

ayumi-signal left a comment

ayumi-signal Nov 8, 2023

ayumi-signal Nov 8, 2023

ayumi-signal Nov 8, 2023 •

edited

Loading

ayumi-signal Nov 8, 2023

bhaskarraksahb commented Nov 10, 2023

ayumi-signal commented Nov 10, 2023

ayumi-signal commented Nov 15, 2023

fixes [6654] Clicking on user avatar (profile picture) in header doesn't open conversation details #6658

fixes [6654] Clicking on user avatar (profile picture) in header doesn't open conversation details #6658

Conversation

bhaskarraksahb commented Nov 5, 2023 • edited Loading

First time contributor checklist:

Contributor checklist:

Description

jamiebuilds-signal commented Nov 6, 2023

jamiebuilds-signal commented Nov 6, 2023

ayumi-signal commented Nov 6, 2023 • edited Loading

bhaskarraksahb commented Nov 7, 2023

bhaskarraksahb commented Nov 8, 2023

ayumi-signal left a comment

Choose a reason for hiding this comment

ayumi-signal Nov 8, 2023

Choose a reason for hiding this comment

ayumi-signal Nov 8, 2023

Choose a reason for hiding this comment

ayumi-signal Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

ayumi-signal Nov 8, 2023

Choose a reason for hiding this comment

bhaskarraksahb commented Nov 10, 2023

ayumi-signal commented Nov 10, 2023

ayumi-signal commented Nov 15, 2023

bhaskarraksahb commented Nov 5, 2023 •

edited

Loading

ayumi-signal commented Nov 6, 2023 •

edited

Loading

ayumi-signal Nov 8, 2023 •

edited

Loading