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

Add the ability to specify rendering options #6873

Merged
merged 12 commits into from
Jun 10, 2020

Conversation

kollivier
Copy link
Contributor

@kollivier kollivier commented May 13, 2020

Summary

For certain renderers, it can be helpful to configure rendering options on a per-ContentNode basis. A common example, included in this PR, is the ability to specify a custom height and width for an HTML5 app. This has been an issue with some in-progress content sources, as well as some content sources we'd like to support in the future. (Like Unity games in portrait or landscape mode.)

This PR creates a ContentNode.options JSON field that allows for the ability to specify renderer-specific options for that node. The ability to specify HTML5 app height and width is included as a part of this PR.

Reviewer guidance

This can't currently be reviewed with content channels because we would need to first add the ability for Studio to export channels with this option. (PR here: learningequality/studio#1896) Once that is done, I can push a version of the ProFuturo content that provides these options for testing.

References


Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles
Copy link
Member

Note: need to add a migration for the change to content models (as the main Kolibri models inherit from the base models).

@rtibbles
Copy link
Member

Oh, but you have added one - Django does not seem to be picking it up for some reason.

@rtibbles
Copy link
Member

Need to update the tests as well, there's a section on this in the documentation in base_models.py - basically need to create a new test class that tests importing from the VERSION_3 schema.

@rtibbles
Copy link
Member

Looks like your latest commit deleted the updated migration, which is causing the test failures.

Code looks correct to me, although I note that the to_sqlite_value change you mentioned to me is not present, which I think is probably needed as you said.

@kollivier
Copy link
Contributor Author

Sorry, I forgot to run git add again on the migration after re-generating it. As for convert_to_sqlite_value, it's weird, I too didn't see it in the PR changes, but after going to the actual commit and seeing the change there, then looking again at all the PR changes, it appears now. 🤷Anyway, hopefully everything is fixed now!

@indirectlylit indirectlylit mentioned this pull request May 26, 2020
9 tasks
@rtibbles rtibbles mentioned this pull request Jun 5, 2020
9 tasks
@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

Merging #6873 into develop will increase coverage by 0.00%.
The diff coverage is 69.23%.

Impacted Files Coverage Δ
..._viewer/assets/src/views/Html5AppRendererIndex.vue 0.00% <0.00%> (ø)
...bri/plugins/learn/assets/src/views/ContentPage.vue 16.49% <ø> (ø)
kolibri/core/content/base_models.py 100.00% <100.00%> (ø)
kolibri/core/content/constants/schema_versions.py 100.00% <100.00%> (ø)
kolibri/core/content/serializers.py 93.93% <100.00%> (+0.02%) ⬆️
kolibri/core/content/utils/channel_import.py 89.04% <100.00%> (+0.12%) ⬆️
kolibri/core/content/utils/sqlalchemybridge.py 84.74% <100.00%> (ø)
kolibri/plugins/facility/views.py 62.50% <0.00%> (-4.17%) ⬇️
kolibri/core/logger/csv_export.py 65.38% <0.00%> (-1.29%) ⬇️
kolibri/core/tasks/utils.py 93.10% <0.00%> (ø)
... and 7 more

@rtibbles
Copy link
Member

rtibbles commented Jun 9, 2020

I think the only thing missing here is the update to https://github.com/learningequality/kolibri-design-system - for the addition of the content mixin.js option prop.

@kollivier
Copy link
Contributor Author

Kolibri design system PR: learningequality/kolibri-design-system#31

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.

Merging this - noting that it will not be fully functional until Kolibri is pointing at the most recent version of KDS.

@rtibbles rtibbles merged commit ddc05eb into learningequality:develop Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants