-
Notifications
You must be signed in to change notification settings - Fork 0
Timeago component refactor #182
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
Conversation
|
||
if (diffInSeconds > TIMEAGO_NOW_THRESHOLD) { | ||
const minutes = Math.round(diffInSeconds / MINUTE_SECONDS); | ||
return withCustomRender(renderMinutes, minutes, `${minutes}m`); |
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.
Should this string (the default ones) be translated maybe? Just a guess.
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 think react-common doesn't handle translations. That's why I thought we need those custom renders to provide translations in specific apps. With this, we can create a wrapper for Timeago for example in Feeds which will handle translations there.
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.
Some components of design-system
handles i18n, so I think it wouldn't be an issue to handle this in react-common
as well. I see that we don't have any components in this repo that translates anything but maybe we should think about that.
} | ||
|
||
// less than a minute ago | ||
return typeof (renderNow) === 'function' ? renderNow() : 'now'; |
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.
Shouldn't this be translated?
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.
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 like that idea. Good customization
display: this.props.datetime, | ||
}; | ||
export default function Timeago({ | ||
datetime, renderNow, renderDate, renderDays, renderHours, renderMinutes, |
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.
Can render*
function return JSX/JS string? Ideally Id like to use renderNow={() => <strong>NOW!</string>}
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.
You can return anything because it will just call your render*
function and return its result. Check this fragment out: return typeof (renderNow) === 'function' ? renderNow() : 'now';
So sure, you can use it like you did it above :)
I noticed that Timeago component had several issues, so I decided to update it because we use it in several places among different projects.
Changes:
datetime
prop - it nows correctly handleDate
object andstring
representing a datedatetime
attribute for<time />
tag to usetoLocaleString()
title
attribute for<time />
minutes
param asnumber
hours
param asnumber
days
param asnumber
date
param asstring
(localDateString
)