Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow deeplinking to a specific Chat message #19221

Open
wants to merge 3 commits into
base: master
from

Conversation

@cjb
Copy link
Contributor

commented Aug 30, 2019

@keybase/react-hackers CC @keybase/picnicsquad @keybase/design

A present for @mmaxim :) Adds a "Copy a link to this message" option to the message dropdown, and URL handler for going to that conversation and highlighting the message.

Perhaps this should be behind a feature flag while we try it out? Since it exposes the deeplinking feature to users, and solidifies the URL format (because people will start keeping and sharing the links).

I plumbed this through selectConversation, because we won't know the conversationID for the linked message until we get to the bottom of selectConversation.

@malgorithms

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

cool feature!

@jakob223
Copy link
Contributor

left a comment

LGTM with one nit; @mmaxim might want to take a look too though.

@@ -414,6 +414,16 @@ export const getParticipantSuggestions = (state: TypedState, id: Types.Conversat
return _getParticipantSuggestionsMemoized(participants, teamType)
}

export const getConversationLabel = (conv: Types.ConversationMeta, includeChannelName: boolean): string => {

This comment has been minimized.

Copy link
@jakob223

jakob223 Sep 3, 2019

Contributor

maybe call the second arg alwaysIncludeChannelName?

@mmaxim mmaxim self-assigned this Sep 3, 2019

@mmaxim
Copy link
Member

left a comment

Don't these two thread loads fight with each other? loadMoreMessages and maybeHighlightMessageID are going to race.

@cjb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@mmaxim How about this, 3fc3e21? I see the sequencing happening properly in the console, though that doesn't guarantee there's no race.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.