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 ability to insert variables into HTML files #2317

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Add ability to insert variables into HTML files #2317

merged 1 commit into from
Jan 10, 2019

Conversation

Floppy
Copy link
Contributor

@Floppy Floppy commented Jan 10, 2019

In our theme, we want to add some course data into meta tags, etc, in a custom required/index.html file.

Currently, in required, the XML files are processed so you can do things like this from https://github.com/adaptlearning/adapt-contrib-spoor/blob/master/required/imsmanifest.xml#L9

 <langstring xml:lang="x-none"><![CDATA[@@course.title]]></langstring>

if we add *.html to the grunt replace job as well as *.xml, we should be able to do the same thing in index.html, meaning that themes can then automatically pull in data from the course. For instance:

 <title><![CDATA[@@course.title]]></title>

This is my first venture into the internals of the Adapt framework, so if there is a way I should be testing these jobs, or otherwise improving my PR, please tell me!

@moloko
Copy link
Contributor

moloko commented Jan 10, 2019

@Floppy thanks for this. I think this is a useful addition and may well help with issues like #1921

There's a guide to contributing to adapt here that's worth a quick read - though really you're not far off, I think the only missing piece is that we normally require a matching Github issue for each PR. It's often worth logging this before you start the work to give people an opportunity to comment on what you'd like to do before you start work - this can help avoid re-work or you doing a lot of work submitting something that actually we don't think should be included.

@Floppy
Copy link
Contributor Author

Floppy commented Jan 10, 2019

🙄 I totally should have read the guide, sorry about that. Want me to open one anyway? I certainly would have opened one if this wasn't so tiny, for discussion in advance :)

@moloko
Copy link
Contributor

moloko commented Jan 10, 2019

@Floppy no, it's all good for this instance, particularly as you have given a comprehensive description of what is is you want to achieve and why

@moloko moloko merged commit a6cc426 into adaptlearning:master Jan 10, 2019
@Floppy Floppy deleted the enhancement/add-html-files-to-grunt-replace branch January 10, 2019 11:49
@Floppy
Copy link
Contributor Author

Floppy commented Jan 10, 2019

Fantastic, thanks. When this is released I'll do a PR into the vanilla theme to add the course title into the header title tag, show how it's used :)

@Floppy
Copy link
Contributor Author

Floppy commented Jan 10, 2019

Having checked the contributor guide first of course 😉

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.

4 participants