Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

feat(Time): Add readout overrides 5app/dashboard#8221 #98

Merged
merged 3 commits into from
Jul 21, 2020

Conversation

MrSwitch
Copy link
Member

For 5app/dashboard#8221 see comments in https://github.com/5app/dashboard/pull/8225#issuecomment-660996737

Adds support for readoutFunctions{}

<Time
dateTime="2019-02-27T12:06:14Z"
readoutFunction={{
      secondsAgoReadout = () => `seconds ago`,
      minutesAgoReadout = () => `minutes ago`,
}}
/>

@MrSwitch MrSwitch requested a review from diondiondion July 21, 2020 11:41
Comment on lines 56 to 59
else if (offset < TIME_MINUTE) {
delay = TIME_MINUTE - offset;
dateString = '< 1 minute ago';
dateString = minutesAgoReadout(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should get rid of this state and just keep going with seconds until the first minute is reached, or alternatively generously round up to a full minute?

Because the way translation libraries usually handle plurals without further customisation means that this will result in the readout "0 minutes ago".

If you want to keep this state, I'd give it its own custom readout getter, to make it very obvious that this third state exists (e.g. lessThanOneMinuteAgoReadout).

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that it can be a little jarring to update regularly. Perhaps we should just pass in the seconds and not show the count?

Copy link
Member Author

@MrSwitch MrSwitch Jul 21, 2020

Choose a reason for hiding this comment

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

@diondiondion i've changed this so that it will call secondsAgoReadout when under a minute. I've set the delay period to <=5 seconds. And set 0 seconds to be 1 second.

So starting from 0 we should expect to see something like "1 second ago", "5 seconds ago", "10 seconds ago", "15 seconds ago", .... "55 seconds ago", "1 minute ago", "2 minutes ago", ... "9 minutes ago", "8:21 pm"

I'm tempted to jump in 10 second intervals. Can play around with it all we want and forever. 😄

Copy link
Member

Choose a reason for hiding this comment

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

@diondiondion i've changed this so that it will call secondsAgoReadout when under a minute. I've set the delay period to <=5 seconds. And set 0 seconds to be 1 second.

So starting from 0 we should expect to see something like "1 second ago", "5 seconds ago", "10 seconds ago", "15 seconds ago", .... "55 seconds ago", "1 minute ago", "2 minutes ago", ... "9 minutes ago", "8:21 pm"

I'm tempted to jump in 10 second intervals. Can play around with it all we want and forever. 😄

That sounds like a good solution. 10 seconds would've probably been fine, but let's just see how it feels in use.

@MrSwitch MrSwitch merged commit 8d2239f into master Jul 21, 2020
@MrSwitch MrSwitch deleted the time-component-update branch July 21, 2020 20:35
5app-Machine added a commit that referenced this pull request Jul 21, 2020
# [11.3.0](v11.2.0...v11.3.0) (2020-07-21)

### Features

* **Time:** Add readout overrides 5app/dashboard[#8221](https://github.com/5app/base5-ui/issues/8221) ([#98](#98)) ([8d2239f](8d2239f))
@5app-Machine
Copy link
Contributor

🎉 This PR is included in version 11.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants