-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Initial support tick messages #226
Initial support tick messages #226
Conversation
@jeromegrosse Why are |
@kfiroo Yup, you are definitely right. I messed up there, I should use the messages array for that. I'll update the pull request with those changes as soon as possible. |
…t and are now read from the message state
@kfiroo I've just pushed a few changes:
What do you think? Here is a screenshot of how it looks like with the changes: |
@jeromegrosse Great! :) |
@@ -52,6 +53,20 @@ export default class Bubble extends React.Component { | |||
return null; | |||
} | |||
|
|||
renderTicks() { | |||
_renderTick = function(displayTick) { |
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.
Creating nested methods is very wasteful and not necasery in this case, so I think _renderTick
should be a method of Bubble, or even as a stateless component.
Also looks like it is a global variable - did you forget the const
?
@jeromegrosse I'm missing a way to override the |
I'll take a look this weekend |
…ested function in renderTicks
@kfiroo I performed the following modifications:
However, I have a couple of questions:
|
renderTicks() { | ||
if (this.props.currentMessage.sent || this.props.currentMessage.received) { | ||
const {currentMessage} = this.props; | ||
if (this.props.renderTicks) { |
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.
This could go above the first if
The user might want to have different props for his sent / received props
@jeromegrosse Great work!
|
@kfiroo @FaridSafi
|
return this.props.renderTicks(currentMessage); | ||
} | ||
|
||
const {currentMessage} = this.props; |
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.
This line should be above the if statement - you are using currentMessage
and it is not defined
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.
Stupid overlook from my part. Nice catch!
@jeromegrosse Yeah, I would move them to the left now :) sorry! |
Have you got time to review it @kfiroo @FaridSafi ? |
@jeromegrosse @FaridSafi looks good to me :) |
…meter tickStyle to GiftedChat
@kfiroo @FaridSafi Just a little update to be able to customize how ticks are displayed (mainly overwriting the colour) by simple passing |
Any update on this? |
@dgdavid @cooperka @scarmuega would love your opinion on this |
Code LGTM, haven't tested on a device though. Nice work. |
The code also looks good to me, but I've some doubts about merging it directly or improve it a little before. I'll try to explain myself. As @cooperka said, this is a I would split ...
renderBoobleFooter={ function() {
renderTime();
renderTicks();
} }
... or change the entire footer passing a completely different logic. As a more detailed example, below you have some of code that I'm using currently in my project: ...
messageFooter(timeProps) {
let icon = null
const { currentMessage, position } = timeProps
const time = moment.tz(currentMessage.createdAt, timezone).format('LT')
const text = <Text style={styles[position].text}>{ time }</Text>
// TODO: improve the icon logic
if (showStatusIcon(currentMessage.status)) {
icon =
<Icon
name={statusIcons[currentMessage.status]}
style={[styles[position].text, styles[position].icon]}
/>
}
return (
<View style={styles[position].container}>
{ text }
{
currentMessage.status === 'FAILED'
? <TouchableOpacity onPress={() => this.onSend([currentMessage])}>{icon}</TouchableOpacity>
: icon
}
</View>
)
}
render() {
return (
<View style={{ flex: 1 }}>
<GiftedChat
style={styles.mainContainer}
messages={this.props.messages && this.props.messages.map(m => unflatten(m))}
onSend={this.onSend}
messageIdGenerator={uuid.v4}
renderTime={this.messageFooter}
user={{
_id: this.props.currentUser.id,
}}
/>
</View>
);
}
}
... I hope that you understand what I trying to say. |
@dgdavid: I agree with all points, though I think most of that can be held off for a future PR. I'm always in favor of merging early with an MVP and iterating from there. This PR is a fairly simple change that allows the possibility of showing send-receipts, and your suggestions seem to me like a larger structural change to the whole component (possibly breaking). I agree it should be done as you suggest, but perhaps in a separate PR after this one? In terms of UX, WhatsApp (just as an example) uses icons on the right of the timestamp. Telegram is the same way. Below is a nice description of what each icon means on WhatsApp, showing how complex this could eventually be. Again, I don't think we need this now, but it's nice to see an example: TL;DR: Let's ship this PR and tweak things later |
@cooperka I also agree with you. I proposed to do it in this PR by the relation with the Thanks for the MVP link 😉 |
@dgdavid @cooperka EDIT: @cooperka this MVP link is indeed a good read |
@jeromegrosse @cooperka @dgdavid Hey guys However, I would like to open a new issue(s?) for discussing the future of
Not so sure if we need a single issue or an issue for each topic but I guess we'll find out soon enough :) |
So, can this PR be merged and later on have other issues improving the feature or are there any work to be done in this PR? |
Friendly ping @kfiroo |
Please push to npm as well |
@jeromegrosse Please add |
Can we add docs of this in front page? |
This feature is already implemented? I couldn't find in the documentation, and I really need that.. Thank you guys! |
Hey everyone any thoughts on the implementation here #644 for doing essentially the same thing but outside of the bubble? The only issue I found is that these two lines need to be commented out following that method
otherwise it will error out with "TypeError: undefined is not a function(evaluating 'this.renderMessageVideo()')". I find it strange though since these both exist on the initial Bubble Component which they are extended from? Any suggestions as to which route to take, and if this latter approach works why these two methods might be failing as undefined? |
Any update about this how we can implement a tick when delever and seen message |
To message object add
|
any update about seen messages and unseen message? and how to implement the isTyping option? |
?? soooo, is seen implemented or not? If so, where is the documentation? |
Re isTyping, use the |
hi am unable to bind messages to giftedchat messages using global list.Iin contructor i declared like global.messagesList=[]; <GiftedChat |
Please, add it into the Readme file |
So, is it possible to indicate that the message was not only delivered but also read? |
I thought it would be a good idea to support ticks appearing in the bubble ala WhatsApp or Telegram depending if the message has been delivered and read.
Here is a screenshot of how it appears as of now with both the read and delivered flag set to
true
:What do you think?