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

Adds props/computed props to mixin for KContentRenderer #224

Merged
merged 6 commits into from
May 19, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Apr 21, 2021

Description

Adds timeSpent, duration props and forceTimeBasedProgress, durationBased computed props so that we can refactor Kolibri content renderers to use unified time-tracking.

Issue addressed

Addresses #214

Steps to test

Currently, to test this PR, you'll have to use learningequality/kolibri#8037 in order to make sure the new props and computed props are working (see tech-debt items below for details).

(optional) Implementation notes

At a high level, how did you implement this?

Added props and computed props to the mixin used by KContentRenderer.

Does this introduce any tech-debt items?

There are currently no tests in KDS to address the use of these props and computed props; however, tests exist on the individual renderers in Kolibri.

Reviewer guidance

  • Is the code clean and well-commented?

Comments

@sairina sairina requested a review from rtibbles April 21, 2021 21:52
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Looks good - thinking it will be useful to implement the solution to learningequality/kolibri#7970 in parallel to this to make sure it has everything it needs?

lib/content/mixin.js Outdated Show resolved Hide resolved
@indirectlylit
Copy link
Contributor

FYI in my PR #223 I added support for extracting JSDocs from the KContentRenderer and displaying them as a normal component:

image

I did not try documenting the props and method yet

sairina added 2 commits April 23, 2021 14:16
…ystem into kcontentrenderer with fix to read epubs
@sairina
Copy link
Contributor Author

sairina commented May 11, 2021

@indirectlylit Nice! I'll make an issue to update this particular component once everything is merged in! Thanks!

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Realized a possible mistake in the original spec while reviewing!

lib/content/mixin.js Outdated Show resolved Hide resolved
lib/content/mixin.js Outdated Show resolved Hide resolved
@sairina sairina marked this pull request as ready for review May 13, 2021 20:31
@indirectlylit
Copy link
Contributor

looks great!

Just a reminder to file an issue for writing prop jsdocs

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