Closed roedoejet closed 7 months ago
PR Preview Action v1.4.7
:---:
:rocket: Deployed preview to https://ReadAlongs.github.io/Web-Component/pr-preview/pr-241/
on branch gh-pages
at 2024-04-02 22:24 UTC
More comments, now that I have tested this feature.
File names:
readalong.zip
would be better than output.zip
, more self-documenting.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.
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?
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.
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.
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.
can the readme contain section about deployment on wordpress, with the apporpiate snippet : ` [wp_read_along_web_app_loader version='^1.2.0']
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.
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
Here's a first stab at this. A few things:
demo.component.ts
~ I can live with this for now, but I think we're due for some refactoringoutput
since if people are combining multiple readalongs they'll have to rename everything. timestamps?assets
folder stuff needs to be improved. We have theuse-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 anassets
folder relative to the location of the ReadAlong, which is sure to break in many deployments. Update: see https://github.com/ReadAlongs/Web-Component/pull/243prettier
on everything. I didn't do that here yet, to make sure the content changes are easily parsable.