-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat(kudos): send kudos by text in standups #9259
Changes from all commits
56fcf95
8a1f3a0
b0520eb
9eaeb3b
c69445d
e09969d
92601b9
606a9a8
cce3c1e
fc59e23
b16b0d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,29 +4,27 @@ import {mapKudosReceivedToToast_notification$data} from '../../__generated__/map | |
import makeNotificationToastKey from './makeNotificationToastKey' | ||
import {OnNextHistoryContext} from '../../types/relayMutations' | ||
import SendClientSideEvent from '../../utils/SendClientSideEvent' | ||
import getReactji from '~/utils/getReactji' | ||
|
||
graphql` | ||
fragment mapKudosReceivedToToast_notification on NotifyKudosReceived { | ||
id | ||
name | ||
meetingName | ||
meetingId | ||
emoji | ||
emojiUnicode | ||
} | ||
` | ||
|
||
const mapKudosReceivedToToast = ( | ||
notification: mapKudosReceivedToToast_notification$data, | ||
{atmosphere, history}: OnNextHistoryContext | ||
): Snack => { | ||
const {id: notificationId, meetingName, name, emoji, meetingId} = notification | ||
const {unicode} = getReactji(emoji) | ||
const {id: notificationId, meetingName, name, emojiUnicode, meetingId} = notification | ||
return { | ||
autoDismiss: 5, | ||
showDismissButton: true, | ||
key: makeNotificationToastKey(notificationId), | ||
message: `${unicode} ${name} gave you kudos in`, | ||
message: `${emojiUnicode} ${name} gave you kudos in`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this missing the meeting name like you used in the other messages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
action: { | ||
label: meetingName, | ||
callback: () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -267,9 +267,12 @@ const getSlackMessageForNotification = async ( | |
const responseId = notification.responseId | ||
const response = await dataLoader.get('teamPromptResponses').loadNonNull(responseId) | ||
const author = await dataLoader.get('users').loadNonNull(response.userId) | ||
const title = notification.kudosEmojiUnicode | ||
? `${notification.kudosEmojiUnicode} *${author.preferredName}* mentioned you and gave kudos in their response in *${meeting.name}*` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 similar to the above, I'd prefer just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nickoferrall The reason it is written this way is that we want to distinguish between kudos that left as a reaction and kudos that is a mention. If I just keep it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be a good one to at least collect internal feedback when both kinds of kudos are in place. I vote for Igor's more verbose but more clear option for now and we can adjust later depending on feedback |
||
: `*${author.preferredName}* mentioned you in their response in *${meeting.name}*` | ||
return { | ||
responseId, | ||
title: `*${author.preferredName}* mentioned you in their response in *${meeting.name}*`, | ||
title, | ||
buttonText: 'See their response' | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Why did you change this? I had used unicode for team health and had to change it to the text representation to avoid issues with analytics. Wouldn't it be better if we kept it like before for kudos emojis? cc @tghanken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tianrunhe You have a little more context over the emoji issues than I do. Did the original implementation only break in BQ? Or were we also seeing issues in places like Amplitude?
Overall, since we're not yet adding the ability to customize the emoji, and we're likely going to revamp how we extract this into BQ early next year, I'm not too worried about this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently store both: text representation and unicode. In this piece of code I just using
emojiUnicode
for rendering, text representation is still available.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. Amplitude can display them fine. It's only BQ that's broken.