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

Update to TimelineJS3 #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

DegrangeM
Copy link

@DegrangeM DegrangeM commented May 12, 2021

};
}

// convert date to TimelineJS3
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to use H5P's ugrades.js functionality to upgrade parameters once when updating content to the new minor version instead of doing that over and over again when the content type runs. I don't think there's a good example, but Interactive Video has quite some history, so this might help as reference: https://github.com/h5p/h5p-interactive-video/blob/master/upgrades.js

Copy link
Author

Choose a reason for hiding this comment

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

I though about using upgrade but I don't think it's a good idea here.

The sementics followed exactly the same structure as the object given to timeline, which allow them to just give the params variable to the timeline library.

However the v3 structure is more complex. It would require 6 fields instead of 2 for each date which will might quickly become ugly. There is also some additionnal depth for some option so this would still require reparsing each date. Also there is some additionnal check required because the timelineJS3 library "crash" if some params are defined as undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. So there then are some options that Timeline 3 offers that you cannot use with H5P? Hmmm. Guess we'll have to wait and see when someone notices that :-)

Copy link
Author

Choose a reason for hiding this comment

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

Yes (https://timeline.knightlab.com/docs/options.html and https://timeline.knightlab.com/docs/json-format.html#json-era).
The display date could solve a request on the forum, but if new functionnality should be added, it should probably be in an other PR aftet this one get merged (if it get merged).

@DegrangeM
Copy link
Author

Is crowdin.yml still necessary ?

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.

2 participants