Add suggested time component with strings #8423
Conversation
nucleogenesis
added
TODO: needs review
| context: | ||
| 'A tooltip explaning what the clock icon and time indicates about the resource they are viewing', | ||
| }, | ||
| shortTime: { |
There was a problem hiding this comment.
@nucleogenesis Is there a way to reuse the existing minute/minutes string from the TimeDuration component?
If I'm understanding correctly, you could even combine the suggestedTimeTooltip message with the existing minutes from TimeDuration component to achieve the purpose of sugestedTimeLabel, and effectively need to stub just one string here 😉
| 'A tooltip explaning what the clock icon and time indicates about the resource they are viewing', | ||
| }, | ||
| shortTime: { | ||
| message: '{time} {time, plural, one {minute} other {minutes}}', |
There was a problem hiding this comment.
These do need to be translated - you're not using any time related syntax here, this is just a simple plural message.
If we use this string and it is not translated, we will end up with minutes being displayed to end users.
There was a problem hiding this comment.
I noticed that and figured that when I come back to it (post strings-getting-in rush) then I'll conditionalize showing any strings at all on whether or not we have > 0 passed in
There was a problem hiding this comment.
Ah - I already had it conditionalized.
@radinamatic - I think that TimeDuration will work the best but it isn't accurate - it rounds down so 2hrs 45min would show "2 hours". If @jtamiace and @rtibbles are okay with the way it rounds, then this component can be the "Suggested time to complete:" followed by the TimeDuration component.
There was a problem hiding this comment.
Understood, thank you @nucleogenesis!
That type of time rounding does not seem like a big concern to me, but will defer to @jtamiace & @rtibbles.
There was a problem hiding this comment.
I think if it's an issue we can probably further conditionalize TimeDuration to round up rather than down?
There was a problem hiding this comment.
It seems okay for now since it still gives a ballpark about expected length, but I'd hope accuracy is something we can improve in the future.
Any issue I can foresee is around planning and scheduling around learning with limited time and the resource taking longer than they expected, but I can't imagine that causing too many issues because it's just an estimate and people will engage resources at their own discernment.
There was a problem hiding this comment.
further conditionalize TimeDuration to round up rather than down
And I totally misread that, and thought that 1:45h was rounded to 2h... 🤦🏽♀️
Yes, rounding up would be more accurate in this case!
There was a problem hiding this comment.
I'd hope accuracy is something we can improve in the future.
We can also conditionalize the TimeDuration component's threshold for when it starts displaying 'hours', so we could make it display whole minutes for up to 120 minutes, and only then start displaying hours. Once we get past 120 minutes, hopefully any time durations are very ballpark anyway.
There was a problem hiding this comment.
I just pushed it up with only the "Suggested time to complete" string.
In follow up I can use TimeDuration to get things looking how we want - but at this point this should be a go for strings I think. Anybody feel free to force push anything you need to get this merged!
There was a problem hiding this comment.
Thanks for the update - and apologies for brigading your tiny PR!
Summary
This is only to add strings and includes minimal functionality.
Adding strings and component for "Suggested Time" frontend feature.
Does not do anything except provide three possible strings.
References
Fixes #8111
#8395
Reviewer guidance
Are these going to work? Do we need different strings for this use case?
Do we need to have it go "X hours Y minutes"?
Testing checklist
PR process
Reviewer checklist
yarnandpip)