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

Add timestamp to the message. Close #16 #17

Merged
merged 18 commits into from
Oct 22, 2017

Conversation

soueuls
Copy link
Contributor

@soueuls soueuls commented Oct 19, 2017

This is my attempt at adding support for timestamp message.
I also added a CSS class so the user can further customize.

This is my first public PR, let me know if there is anything I can do.

Copy link
Owner

@andersonba andersonba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, man!
Thanks for your contribution 🤖

Check my comments, please 👍

package.json Outdated
@@ -53,5 +53,8 @@
"!src/core/index.ts",
"!src/ui/index.ts"
]
},
"dependencies": {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moment is an awesome lib to manipulate date, but it's a heavy lib.
Thinking of keeping the yvebot more customizable as possible, I'd rather leave it for the user to choose.

You can use a formatter function from options, allowing the user to override it:

new YveBot(..., {
  timestampable: true,
  timestampFormatter: ts => moment(ts).fromNow(),
})

I would create a simple formatter as default:

formatTimestamp = ts => new Date(ts).toUTCString().slice(-12, -4);
// 00:19:43

src/ui/bot.ts Outdated
const sender = source === 'BOT' ? this.options.name : null;
const thread = UI.createThread(source, UI.createTextMessage(message, sender));
const thread = UI.createThread(source, UI.createTextMessage(message, sender, showTimestamp));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the UI instance has access to options object, so, it's unnecessary to pass as arg [comment below]

src/ui/ui.ts Outdated
@@ -179,7 +180,7 @@ export class ChatUI {
conversation.scrollTop = conversation.scrollHeight;
}

createTextMessage(answer: Answer | Answer[], senderName?: string) {
createTextMessage(answer: Answer | Answer[], senderName?: string, showTimestamp?: boolean) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the arg

src/ui/ui.ts Outdated
@@ -204,6 +205,13 @@ export class ChatUI {
message.innerHTML = text;
bubble.appendChild(message);

if (showTimestamp) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract it from options

const { ... } = this.options;

src/ui/bot.ts Outdated
@@ -19,6 +19,7 @@ export class YveBotChat {
doneMultipleChoiceLabel: 'Done',
andSeparatorText: 'and',
submitLabel: 'Send',
showTimestamp: true,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make default as false

src/types.ts Outdated
@@ -49,6 +49,7 @@ export interface ChatOptions {
doneMultipleChoiceLabel?: string;
andSeparatorText?: string;
submitLabel?: string;
showTimestamp?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timestampable

@andersonba
Copy link
Owner

Fix the tests, also, create one to ensure this new feature

@soueuls
Copy link
Contributor Author

soueuls commented Oct 20, 2017

Thank you for the feedback ! I will happily work on this tonight or tomorrow, i definitely like the date formatter idea better.

@andersonba
Copy link
Owner

Awesome, @soueuls 🤘
I need you to rebase your branch with master to merge this PR. Remember to re-build the lib.

# This is the 1st commit message:

Add timestamp to the message.

# This is the commit message andersonba#2:

Timestamp feature is now generic + tests
@soueuls
Copy link
Contributor Author

soueuls commented Oct 21, 2017

I rebased from master, build the files.
But then I messed things up when I tried to squash the commits, I hope you can fix the historic to make it looks a bit nicer.

I am still learning few things with git 🤕

@andersonba
Copy link
Owner

hahaha that's ok, no problem. I will squash and merge your PR.
Check the tests, there is a broken test.
You need to mock the date using jest.useFakeTimers, see here.

@andersonba andersonba merged commit 403ea8e into andersonba:master Oct 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants