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

Changed the copyright modification to the respec-based cc statement #2549

Merged
merged 5 commits into from
Mar 30, 2023

Conversation

iherman
Copy link
Member

@iherman iherman commented Mar 29, 2023

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

@mattgarrish
Copy link
Member

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?

@mattgarrish
Copy link
Member

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.

@iherman
Copy link
Member Author

iherman commented Mar 29, 2023

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...

@mattgarrish
Copy link
Member

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.

@iherman
Copy link
Member Author

iherman commented Mar 29, 2023

Oops, I missed the data-oninclude feature. That may be indeed better...

@mattgarrish
Copy link
Member

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 and others added 4 commits March 30, 2023 10:55
* Use respec includes for copyright statements;
remove calls to fixIncludes

* fix conflict in js

* remove copyright function from pre/post processing calls

* remove empty postprocess calls

* test div includes

* test no oninclude script

* undo tests

* test no script

* no copyright

* copyright bottom

* undo

* add missing include file
@iherman
Copy link
Member Author

iherman commented Mar 30, 2023

@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.

@mattgarrish
Copy link
Member

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.

@mattgarrish mattgarrish merged commit 017ea47 into main Mar 30, 2023
@mattgarrish mattgarrish deleted the copyright-change-via-respec branch March 30, 2023 12:41
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.

None yet

2 participants