w3c / ttml2

Timed Text Markup Language 2 (TTML2)
https://w3c.github.io/ttml2/
Other
40 stars 16 forks source link

fix radii example #1251

Closed nigelmegitt closed 1 year ago

nigelmegitt commented 1 year ago

Brings in @MarcAntoine-Arnaud 's fix and closes #1250.

github-actions[bot] commented 1 year ago

Preview | diff

nigelmegitt commented 1 year ago

@himorin I just noticed the diff link doesn't actually show a diff.

himorin commented 1 year ago

@himorin I just noticed the diff link doesn't actually show a diff.

it seems, script from htmldiff does not work correctly with htmlpreview, since diff-ed section exists in pushed diff.html https://htmlpreview.github.io/?https://github.com/w3c/ttml2/blob/issue-1250-border-radii-example-build/diff.html#style-attribute-border-example-2

nigelmegitt commented 1 year ago

diff-ed section exists in pushed diff.html

Does that mean we just need to change the URL in the Diff link?

himorin commented 1 year ago

Looking into htmldiff.js in detail, script only supports (to be exact, only processes) JS files from raw.github.com or bitbucket.org, or inline JS. https://github.com/htmlpreview/htmlpreview.github.com/blob/787b673198b8f13ac16384325cd380c5c68cf219/htmlpreview.js#L47

So, https://w3c.github.io/htmldiff-nav/index.js script for adding htmldiff navigator controls will not run with htmlpreview, and it is correct and the assumed behavior...

nigelmegitt commented 1 year ago

OK, but how did you generate https://htmlpreview.github.io/?https://github.com/w3c/ttml2/blob/issue-1250-border-radii-example-build/diff.html#style-attribute-border-example-2 ? Can we systematize that approach?

himorin commented 1 year ago

OK, but how did you generate

mm? I've got that link from github-bot's post https://github.com/w3c/ttml2/pull/1251#issuecomment-1256076790

for hash (#style-attribute-border-example-2), I've just copied a link of the nearest anchor from changed part. Checked diff in PR and searched in displayed htmldiff.

nigelmegitt commented 1 year ago

OH! I see what's happening now. The diff link does actually work, but I was confused because the component that normally appears at the bottom right of PR Preview diffs, that allows you to step through the differences, does not appear. So in fact the diff link does work, but is not so useful.

I wonder how that "step through the differences" widget works, and if we can add it somehow...

himorin commented 1 year ago

I wonder how that "step through the differences" widget works, and if we can add it somehow...

Wondering around that for a while, I think we might have two possibilities, like

nigelmegitt commented 1 year ago

They both sound good to me - the second option is more within our control, so easier to get going more quickly, I think?

himorin commented 1 year ago

They both sound good to me - the second option is more within our control, so easier to get going more quickly, I think?

test PR at https://github.com/himorin/ttml2/pull/4 diff for GHAction: https://github.com/w3c/ttml2/commit/91c934e0097526006a5e90534917e4092a610aba

PR to w3c/ttml2 opened as https://github.com/w3c/ttml2/pull/1253