feat: add web bundle as download option #241

roedoejet · 2024-03-14T19:02:24Z

Here's a first stab at this. A few things:

The web preview should work if you want to try it out (select "web bundle" from the download format options)
~~I don't love how much logic is in the demo.component.ts~~ I can live with this for now, but I think we're due for some refactoring
Maybe we should come up with a better basename than just output since if people are combining multiple readalongs they'll have to rename everything. timestamps?
The assets folder stuff needs to be improved. We have the use-assets-folder property, but the value is just a boolean. I think we should instead have that be a path that could get prepended to all images, because right now, the images are hardcoded in the .readalong file as being in an assets folder relative to the location of the ReadAlong, which is sure to break in many deployments. Update: see fix(assets)!: get rid of useAssetsFolder boolean and replace with imageAssetsFolder string #243
looks like we need to run prettier on everything. I didn't do that here yet, to make sure the content changes are easily parsable.
Translations for french and spanish still have to be added

github-actions · 2024-03-14T19:05:46Z

PR Preview Action v1.4.7
🚀 Deployed preview to https://ReadAlongs.github.io/Web-Component/pr-preview/pr-241/
on branch `gh-pages` at 2024-04-02 22:24 UTC

joanise · 2024-03-15T19:16:10Z

More comments, now that I have tested this feature.

File names:

readalong.zip would be better than output.zip, more self-documenting.
Ditto the files in assets/ in the archive, assets/readalong.{mp3,readalong} would be better than assets/output.*.
output.txt is confusing, since it's really the input text. readalong.txt would be OK, or maybe input.txt.

The README.txt file should be outdented against the left margin. My guess is you have it indented probably because of the file where you're defining its contents, but I don't like the result, it ought to start on the first line (remove the blank line) and have no indents. It should also be wrapped at <=79 characters. I know we don't do that anymore in code, but here when I double clicked on the file the display was awful at 80 chars wide.

I would also be OK with no manual wrapping at all, letting the auto-wrapping do its thing, but pre-wrapped at 107 yields poor results if the window is even slightly narrower. Between not pre-wrapped at all, and pre-wrapped at 79, I can't decide what I prefer: not pre-wrapped adjusts to whatever window size the user has, 79 is old machine friendly, though maybe not mobile friendly... You pick!
Key is, I'd like the default display to be easy to read on all OSes, machine ages, and user defaults.

joanise · 2024-03-15T19:19:56Z

I tested unzipping the file on my machine and running python -m http.server, and that works nicely.

One thing, though, this is a zip bomb, with no inner folder. Would it make sense to put all the contents in a readalong/ folder inside the zip file? Or is it just because I'm a developer that I'm allergic to tar bombs?

packages/studio-web/src/app/demo/demo.component.ts

joanise · 2024-03-15T19:31:48Z

packages/studio-web/src/app/demo/demo.component.ts

@@ -27,6 +35,7 @@ export class DemoComponent implements OnDestroy, OnInit {
  };
  outputFormats = [
    { value: "html", display: $localize`Offline HTML` },
+    { value: "zip", display: $localize`Web Bundle` },


Zipped Web Bundle?
That would give me "Fichiers Web zippés" and "Archivos web comprimidos".
I find just "Web Bundle" insufficiently clear.

I did some research on "zippés" vs "comprimés" for the French translation, and Google tells me the former is much more common than the latter, and that matches my intuition, but Spanish doesn't seem to have that, it would have to be "Archivos web comprimidos zip" if we wanted to be precise, but I think that's unnecessary, adding more verbosity with no additional clarity, whereas the French "zippés" is shorter and more common, thus a no brainer.

compressés: https://vitrinelinguistique.oqlf.gouv.qc.ca/fiche-gdt/fiche/8399261/compresser

mais même selon l'OQLF "zippé" est acceptable dans certains contextes ;-)

I'm not going to be picky, but my first choice is "zippé" because we're creating an actual .zip file.

Does anyone have an opinion on the English, though, whether we should add "Zipped" in front?

I think "zipped" is redundant - Bundle implies that it's bundled/zipped which is sufficient

joanise · 2024-03-15T19:46:34Z

By the way, we already applied prettier:

$ npx prettier -c packages
Checking formatting...
All matched files use Prettier code style!

the generated index.html file is not prettier'd, though, that's true.

dhdaines · 2024-03-16T13:38:49Z

The README.txt file should be outdented against the left margin. My guess is you have it indented probably because of the file where you're defining its contents, but I don't like the result, it ought to start on the first line (remove the blank line) and have no indents. It should also be wrapped at <=79 characters. I know we don't do that anymore in code, but here when I double clicked on the file the display was awful at 80 chars wide.

black wraps at "90-ish" so we do this in code, more or less. I think it is reasonable to wrap at 80 (if you use Emacs it is as simple as typing Esc-q) as this makes the text easier to read in general (there's a reason why LaTeX makes wide margins). If you don't wrap lines then not everbody's terminal/browser/whatever is going to actually word wrap correctly in any case.

dhdaines · 2024-03-16T13:43:10Z

One thing, though, this is a zip bomb, with no inner folder. Would it make sense to put all the contents in a readalong/ folder inside the zip file? Or is it just because I'm a developer that I'm allergic to tar bombs?

Yes, I think this is a good idea - MacOS or Windows are going to unpack it in a new folder anyway, but this way nobody gets any nasty surprises.

dhdaines

Thank you! Looks good, mostly style comments.

packages/studio-web/src/app/demo/demo.component.ts

packages/studio-web/src/app/soundswallower.service.spec.ts

packages/studio-web/src/app/upload.service.ts

packages/studio-web/src/app/upload/upload.component.ts

packages/studio-web/src/app/demo/demo.component.ts

joanise

Looks good to me now.

dhdaines

looks good! sorry for the delay in approving!

deltork · 2024-03-28T15:24:14Z

can the readme contain section about deployment on wordpress, with the apporpiate snippet :
[wp_read_along_web_app_loader version='^1.2.0'] <read-along...></readalong> [/wp_read_along_web_app_loader]

joanise · 2024-03-28T16:04:19Z

can the readme contain section about deployment on wordpress, with the apporpiate snippet :
[wp_read_along_web_app_loader version='^1.2.0'] <read-along...></readalong> [/wp_read_along_web_app_loader]

Absolutely, just add that in your PR.

deltork · 2024-04-02T15:50:07Z

can the readme contain section about deployment on wordpress, with the apporpiate snippet :
[wp_read_along_web_app_loader version='^1.2.0'] <read-along...></readalong> [/wp_read_along_web_app_loader]

Absolutely, just add that in your PR.

The readme is only in this branch. I will have to add the instructions after this branch is merged

deltork · 2024-04-02T16:01:31Z

packages/studio-web/src/app/demo/demo.component.ts

+        .toISOString()
+        .replace(/[^0-9]/g, "")
+        .slice(0, -3);
+      const basename = `readalong-${timestamp}`;


When people provide a title, adding the title to the filename will help them find it after download, especially if they do multiple books.
const basename = (this.slots.title?this.slots.title.replace(/\s/g,"_").toLowerCase():'readalong')+`-${timestamp}`;

joanise · 2024-04-02T21:25:44Z

packages/studio-web/src/app/demo/demo.component.ts

+        .replace(/[^0-9]/g, "")
+        .slice(0, -3);
+      const basename =
+        (this.slots.title ? slugify(this.slots.title, 15) : "readalong") +


This is great, except it's too anglocentric: if the title is ᐃᓄᒃᑎᑐᑦ, the slug is empty and you get -20240402212031.zip, and the .txt file inside is similarly limited. Similarly "Éric" becomes "ric", but the Inuktitut example is a lot more dramatic. Can we slugify the text-unidecode output, maybe?

thanks for this @joanise - I've updated it to use unidecode

- Update $ style for rxjs variables - Add inner folder for zip - Use timestamps for filenames - Remove indentation for readme - Added explicit version for unpkg import - Added error handling for zip download promise

…geAssetsFolder string BREAKING CHANGE: useAssetsFolder is no longer valid. The equivalent is imageAssetsFolder='assets/' which is the default.

roedoejet marked this pull request as draft March 14, 2024 19:02

roedoejet force-pushed the dev.ap/zip branch 2 times, most recently from 153d2cc to ef8fa1c Compare March 14, 2024 19:22

roedoejet marked this pull request as ready for review March 14, 2024 19:26

roedoejet requested review from joanise and dhdaines March 14, 2024 19:26

joanise reviewed Mar 15, 2024

View reviewed changes

packages/studio-web/src/app/demo/demo.component.ts Outdated Show resolved Hide resolved

joanise reviewed Mar 15, 2024

View reviewed changes

dhdaines requested changes Mar 16, 2024

View reviewed changes

roedoejet requested review from joanise, dhdaines and deltork March 19, 2024 00:47

roedoejet force-pushed the dev.ap/zip branch from f194def to e2e5007 Compare March 19, 2024 00:51

joanise requested changes Mar 19, 2024

View reviewed changes

packages/studio-web/src/app/demo/demo.component.ts Outdated Show resolved Hide resolved

packages/studio-web/src/app/demo/demo.component.ts Outdated Show resolved Hide resolved

roedoejet requested a review from joanise March 19, 2024 16:39

joanise approved these changes Mar 20, 2024

View reviewed changes

dhdaines approved these changes Mar 26, 2024

View reviewed changes

deltork approved these changes Apr 2, 2024

View reviewed changes

roedoejet force-pushed the dev.ap/zip branch 2 times, most recently from 38a0c7c to 0714343 Compare April 2, 2024 20:22

joanise reviewed Apr 2, 2024

View reviewed changes

roedoejet and others added 11 commits April 2, 2024 15:17

feat: add web bundle as download option

38e99de

fix: include review-recommended fixes

05292e9

- Update $ style for rxjs variables - Add inner folder for zip - Use timestamps for filenames - Remove indentation for readme - Added explicit version for unpkg import - Added error handling for zip download promise

fix: use human readable timestamp

63e2d69

docs(i18n): add es and fr translations

a606b6e

docs(i18n): translate the new message "download failed"

16a0cb2

feat: use slot title as file basename in bundle

79bbc0f

feat: use unidecode to create filenames

576953c

fix(assets)!: get rid of useAssetsFolder boolean and replace with ima…

3735e7b

…geAssetsFolder string BREAKING CHANGE: useAssetsFolder is no longer valid. The equivalent is imageAssetsFolder='assets/' which is the default.

fix: check that assets folder ends with /

3788c8f

feat: bump to v1.2.0

f0aefb5

chore: bump single file bundle version

fcc2255

roedoejet force-pushed the dev.ap/zip branch from c674da7 to fcc2255 Compare April 2, 2024 22:21

roedoejet merged commit 2df8ef0 into main Apr 2, 2024
2 checks passed

roedoejet deleted the dev.ap/zip branch April 2, 2024 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add web bundle as download option #241

feat: add web bundle as download option #241

roedoejet commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

joanise commented Mar 15, 2024

joanise commented Mar 15, 2024 •

edited

Loading

joanise Mar 15, 2024 •

edited

Loading

joanise Mar 15, 2024

dhdaines Mar 16, 2024

dhdaines Mar 16, 2024

joanise Mar 19, 2024

roedoejet Mar 19, 2024

joanise commented Mar 15, 2024

dhdaines commented Mar 16, 2024

dhdaines commented Mar 16, 2024

dhdaines left a comment

joanise left a comment

dhdaines left a comment

deltork commented Mar 28, 2024

joanise commented Mar 28, 2024

deltork commented Apr 2, 2024

deltork Apr 2, 2024 •

edited

Loading

joanise Apr 2, 2024

roedoejet Apr 2, 2024

feat: add web bundle as download option #241

feat: add web bundle as download option #241

Conversation

roedoejet commented Mar 14, 2024 • edited Loading

github-actions bot commented Mar 14, 2024 • edited Loading

joanise commented Mar 15, 2024

joanise commented Mar 15, 2024 • edited Loading

joanise Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

joanise Mar 15, 2024

Choose a reason for hiding this comment

dhdaines Mar 16, 2024

Choose a reason for hiding this comment

dhdaines Mar 16, 2024

Choose a reason for hiding this comment

joanise Mar 19, 2024

Choose a reason for hiding this comment

roedoejet Mar 19, 2024

Choose a reason for hiding this comment

joanise commented Mar 15, 2024

dhdaines commented Mar 16, 2024

dhdaines commented Mar 16, 2024

dhdaines left a comment

Choose a reason for hiding this comment

joanise left a comment

Choose a reason for hiding this comment

dhdaines left a comment

Choose a reason for hiding this comment

deltork commented Mar 28, 2024

joanise commented Mar 28, 2024

deltork commented Apr 2, 2024

deltork Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

joanise Apr 2, 2024

Choose a reason for hiding this comment

roedoejet Apr 2, 2024

Choose a reason for hiding this comment

roedoejet commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024 •

edited

Loading

joanise commented Mar 15, 2024 •

edited

Loading

joanise Mar 15, 2024 •

edited

Loading

deltork Apr 2, 2024 •

edited

Loading