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

fix: cannot download other formats #351

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Conversation

deltork
Copy link
Collaborator

@deltork deltork commented Oct 10, 2024

PR Goal?

bug fix

Fixes?

#347

Feedback sought?

sanity check

Priority?

high

Tests added?

to be added to studio test suites

How to test?

use the pr-preview editor to upload an offline format and try to export non web formats

Confidence?

Version change?

bug fix

This PR depends on the merging of PR ReadAlongs/Studio#247

@deltork deltork linked an issue Oct 10, 2024 that may be closed by this pull request
Copy link

semanticdiff-com bot commented Oct 10, 2024

Review changes with SemanticDiff.

Analyzed 1 of 1 files.

Overall, the semantic diff is 41% smaller than the GitHub diff.

Filename Status
✔️ packages/studio-web/src/app/editor/editor.component.ts 40.86% smaller

Copy link
Contributor

github-actions bot commented Oct 10, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-10-11 15:35 UTC

@joanise
Copy link
Member

joanise commented Oct 10, 2024

This looks safe to merge already, even before we merge ReadAlongs/Studio#247
I'm not sure why it's labelled draft, it seems to work correctly already. Given it would fix the live deployment, I'm tempted to merge it ASAP, @roedoejet if you have a chance to review it too maybe we could do that soon, even while Del is away.

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@roedoejet
Copy link
Collaborator

This looks safe to merge already, even before we merge ReadAlongs/Studio#247 I'm not sure why it's labelled draft, it seems to work correctly already. Given it would fix the live deployment, I'm tempted to merge it ASAP, @roedoejet if you have a chance to review it too maybe we could do that soon, even while Del is away.

I don't understand parts of this PR, but I unfortunately don't have enough time to dig into it either. Del is away for the weekend? If you're confident in these changes, go for it, @joanise otherwise my in-depth review would have to wait until next week

@joanise
Copy link
Member

joanise commented Oct 10, 2024

I don't understand parts of this PR, but I unfortunately don't have enough time to dig into it either. Del is away for the weekend? If you're confident in these changes, go for it, @joanise otherwise my in-depth review would have to wait until next week

Fair enough. I guess I'll test more, to make sure it really does work as I think it does, and decide what to do based on that.

Copy link
Member

@joanise joanise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, carefully tested, I'm going to merge this now, deploy it, and even release 1.5.1 with it because I think it's an important and urgent patch.

Comment on lines +267 to +273
if (textNode?.querySelector("body") == null) {
while (textNode?.hasChildNodes()) {
// @ts-ignore
body.appendChild(textNode.firstChild);
}
textNode.appendChild(body);
}
textNode?.appendChild(body);
} /**/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say I don't understand how this code works, but I tested it carefully, and what I can see is that it does work. I took a readalong, changed its body tag to something else in the base64-encoded .readalong file, and this code now correctly insert a new body tag in the right place. Whereas the old code did not, and causes trouble as we know.

@joanise joanise marked this pull request as ready for review October 11, 2024 15:33
@joanise joanise merged commit cd84f13 into main Oct 11, 2024
2 checks passed
@joanise joanise deleted the dev.del/347-download-issues branch October 11, 2024 15:34
joanise pushed a commit that referenced this pull request Oct 16, 2024
* fix: cannot download other formats

* fix: remove xmlns from tags

Fixes #347
joanise pushed a commit that referenced this pull request Oct 16, 2024
* fix: cannot download other formats

* fix: remove xmlns from tags

Fixes #347
joanise pushed a commit that referenced this pull request Oct 17, 2024
* fix: cannot download other formats

* fix: remove xmlns from tags

Fixes #347
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.

Cannot download Elan, Praat, SRT or WebVTT from Editor
3 participants