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.

Sign up for GitHub

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

Jump to bottom

Add suggested time component with strings #8423

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Add suggested time component with strings #8423

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2009557
Add suggested time component with strings
nucleogenesis Sep 13, 2021
b0194a1
Make sure the right string is there
nucleogenesis Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 48 additions & 0 deletions kolibri/plugins/learn/assets/src/views/SuggestedTime.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<template>

<div class="time-context">
<div v-if="time > 0">
{{ $tr('shortTime', { time: 120 }) }}
</div>
<div v-if="time > 0">
{{ $tr('suggestedTimeLabel', { time: 120 }) }}
</div>
{{ $tr('suggestedTimeTooltip') }}
</div>

</template>


<script>

export default {
name: 'SuggestedTime',
props: {
/* Time in ms */
time: {
type: Number,
default: 0,
},
},
$trs: {
suggestedTimeTooltip: {
message: 'Suggested time to complete',
context:
'A tooltip explaning what the clock icon and time indicates about the resource they are viewing',
},
shortTime: {
Copy link
Member

Choose a reason for hiding this comment

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

@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 😉

message: '{time} {time, plural, one {minute} other {minutes}}',
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, thank you @nucleogenesis!
That type of time rounding does not seem like a big concern to me, but will defer to @jtamiace & @rtibbles.

Copy link
Member

Choose a reason for hiding this comment

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

I think if it's an issue we can probably further conditionalize TimeDuration to round up rather than down?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the update - and apologies for brigading your tiny PR!

context: 'Converts the time given into internationalized time. Nothing to translate here.',
},
suggestedTimeLabel: {
message: 'Suggested time to complete: {time} {time, plural, one {minute} other {minutes}}',
context:
"Indicates to the user the suggested time the resource should take to complete. The '# minutes' string is internationalized automatically",
},
},
};

</script>


<style scoped lang="scss"></style>