Closed iherman closed 1 year ago
Do you think it would be easier to put the copyright text in a file under /common and use respec to import it, like we do with the acknowledgements? It's the same for every document, right?
Merge this one first and then the other PR-s, or do the others and then this one?
I'd probably merge the older ones first because once you merge this there will probably be conflicts removing the preprocess function from the older ones. I don't expect the old ones will bring forward any conflicts.
Do you think it would be easier to put the copyright text in a file under /common and use respec to import it, like we do with the acknowledgements? It's the same for every document, right?
Hm. I see one possible problem. The script runs now as a 'preProcess' script, which means before respec does anything. If we used the import feature, it would have to run as a 'postProcess', which was the case previously. And I do not know whether you saw that, but the previous version of the script was terribly slow to run, it was noticeable.
It may be, though, that the reason for the speed difference was the fact that the process of finding the right target for modification was different. In the new version, because I could set the id, the javascript used the getElementById
method, whereas in the previous version I had no other choice then tap into what respec produced, and that involved the javascript version of the css query call. The latter is probably way slower...
I will do some experimentation. Tomorrow...
Since the JS is just tweaking the data, can't that be run as part of the include, though?
I was thinking we'd have imports like this:
<p data-include="../common/copyright.html" data-oninclude="modifyCopyright" data-include-replace="true"></p>
But I'll try testing it and see what happens.
Oops, I missed the data-oninclude
feature. That may be indeed better...
Takes a little tweaking of the copyright and function, but I was able to get it working. I had to read the documentation on data-oninclude to figure out that you have to do content.replace() and add three parameters to the function call, as it wasn't finding the span by id. Basically the function becomes:
function modifyCopyright(utils, content, url) {
// find the place to put the current year
return content.replace("%thisyear%", (new Date()).getFullYear());
}
And in the copyright change the span to a %thisYear%:
... 1999-%thisyear%
I'll open a separate PR against this one so you can try it out before we do anything.
Do you know why our current includes call a "fixIncludes" function, though? Is it something old, perhaps? I can't find a matching function and the respec code unhelpfully doesn't complain if functions don't exist. I'm thinking it would good to remove those function calls if they aren't doing anything.
@mattgarrish I have merged (via squash and merge) your version into this one, and I also removed some leftover 'fixInclude' references.
I did not know whether you preferred to merge the other two, remaining PR-s before processing this one, so I did not merge it. At this moment, it would be free to go.
I think we're fine merging this now. The one PR is stuck until we can figure out the idpf.org errata question and the other is only for a note and may still need some tweaks. This will finalize the three REC docs, which is most important.
There is still a small JS running to set today's year, but it is done as a preprocess and not postprocess and is therefore significantly faster.
@mattgarrish, I am not sure what the optimal order is. Merge this one first and then the other PR-s, or do the others and then this one? We may get some merge conflicts no matter what...
Preview | Diff