fix(assets)!: get rid of useAssetsFolder boolean and replace with imageAssetsFolder string by roedoejet · Pull Request #243 · ReadAlongs/Studio-Web

roedoejet · 2024-03-19T00:44:19Z

BREAKING CHANGE: useAssetsFolder is no longer valid.

The equivalent is imageAssetsFolder='assets/' which is the default.

It used to be that all readalong and audio files were forced to be in the same assets folder along with the images. Then we removed that requirement, and toggled the image assets as a boolean. But, we should really go one step further and define an imageAssetsFolder property on the readalong so that the .readalong file can just define image names and then prefix them with the imageAssetsFolder otherwise we'll be asking users to edit the .readalong file when they deploy their readalongs.

IMPORTANT NOTE: we need to publish v 1.2.0 to npm before merging this to main, otherwise the bundles that are downloaded from studio will incorrectly point to v1.1.0. Before merging this, please ping me to publish the new version on npm!

Also, I don't think the deploy preview will really work here for the bundle/single file html etc, since v1.2.0 won't be published to unpkg yet

github-actions · 2024-03-19T00:47:31Z

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

joanise

Good idea to bump the versions for this breaking change.
I'm good with these changes, modulo not requiring the slash.

joanise · 2024-03-19T19:12:22Z

-   * Toggle the use of assets folder for resolving urls. Defaults to on
-   * to maintain backwards compatibility
+   * Toggle the use of an assets folder. All image paths
+   * will be prefixed with this. Defaults to 'assets'.


Not quite accurate, it actually defaults to 'assets/', with the slash.

In fact, since you do this.imageAssetsFolder + path; in urlTransform(), the slash is mandatory. That should be documented if it stays that way, but in fact I would much rather add it automatically if it's not there rather than require it.

joanise · 2024-03-19T19:17:38Z

-    )
-      return "assets/" + path;
+    ) {
+      return this.imageAssetsFolder + path;


As mentioned above, that's a guaranteed gotcha. The intuitive thing to do in the read-along element is image-assets-folder="images" and that's going to generate e-mail enquiries for support.
I'd rather implement the logic

if folder is empty or undefined return path

else return folder + "/" + path
This else logic is safe even if the user adds the slash, because doubling the slash won't break the URL.

In async componentWillLoad() you can check for the trailing slash and add if needed
if (!this.imageAssetsFolder.endsWith("/")){ this.imageAssetsFolder+="/" }

deltork

I do not think this will be a breaking change since the asset folder default is the same.

I love the idea of passing a folder to the component! no more hard coding URL in .readalong file for those with custom deployments

deltork · 2024-03-27T21:40:13Z

   */

-  @Prop() useAssetsFolder: boolean = true;
+  @Prop() imageAssetsFolder: string = "assets/";


make it mutable @Prop({mutable: true}) imageAssetsFolder: string = "assets/";
so that you can append the trailing slash if missing

deltork · 2024-03-27T21:46:20Z

-    )
-      return "assets/" + path;
+    ) {
+      return this.imageAssetsFolder + path;


In async componentWillLoad() you can check for the trailing slash and add if needed
if (!this.imageAssetsFolder.endsWith("/")){ this.imageAssetsFolder+="/" }

deltork · 2024-03-27T21:47:56Z

  private urlTransform(path: string): string {
    if (
-      this.useAssetsFolder &&
+      this.imageAssetsFolder &&


Not necessary since this.imageAssetsFolder will always exist

Is that still going to be true if the html file specifies images-assets-folder=""?

If we append / to the this.imageAssetsFolder bit, then images-assets-folder="" would effectively become images-assets-folder="/". A user can also do images-assets-folder=".' equivalent to images-assets-folder="./".

But that would be a bug, no? If it's "", we should not add / in front.

I agree; however, it is an input error. So we need to define how to handle it. If a user provides image-assets-folder="" it be treated as "./" or "/". I vote "./"

joanise · 2024-03-28T12:48:07Z

I just tested this PR, and the Web Bundle I got had

<script type="module" src='https://unpkg.com/@readalongs/web-component@^1.1.1/dist/web-component/web-component.esm.js'></script>

where I believe it will need ^1.2.0.

We have something a little tricky here: when Studio-Web and Studio-CLI start producing snippets with ^1.2.0, that will be broken until we publish that on npmjs.

Should we pause auto-updating github-pages from all pushes to main here until we publish web-component 1.2.0 to npmjs?

We're entering an unstable dev stage, that may be the safest way to go. We could change publish.yml to run on push to main_disabled in this PR and revert that change when everything is ready.

joanise · 2024-03-28T12:50:00Z

We're entering an unstable dev stage, that may be the safest way to go. We could change publish.yml to run on push to main_disabled in this PR and revert that change when everything is ready.

Or maybe a better solution is to merge to a dev branch for now, and put all breaking changes there until we're ready instead of in main.

joanise

Looks great!

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

roedoejet force-pushed the dev.ap/img-assets branch from 875fcf5 to 8e254d2 Compare March 19, 2024 00:45

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

roedoejet mentioned this pull request Mar 19, 2024

feat: add web bundle as download option #241

Merged

6 tasks

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

roedoejet force-pushed the dev.ap/img-assets branch 3 times, most recently from 82059bc to 6416c8d Compare March 19, 2024 16:38

joanise requested changes Mar 19, 2024

View reviewed changes

roedoejet force-pushed the dev.ap/img-assets branch from 6416c8d to 3dd279b Compare March 20, 2024 17:15

deltork requested changes Mar 27, 2024

View reviewed changes

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

roedoejet force-pushed the dev.ap/img-assets branch from 3dd279b to 63b5814 Compare April 2, 2024 20:23

joanise approved these changes Apr 2, 2024

View reviewed changes

roedoejet requested review from deltork and joanise April 2, 2024 21:35

roedoejet added 2 commits April 2, 2024 15:04

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

39ebd5f

…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 /

2688bf0

roedoejet force-pushed the dev.ap/img-assets branch from 63b5814 to 2688bf0 Compare April 2, 2024 22:04

feat: bump to v1.2.0

3b626b5

roedoejet merged commit 2c55d86 into dev.ap/zip Apr 2, 2024

roedoejet deleted the dev.ap/img-assets branch April 2, 2024 22:13

Conversation

roedoejet commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joanise left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltork left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joanise commented Mar 28, 2024

Uh oh!

joanise commented Mar 28, 2024

Uh oh!

joanise left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

roedoejet commented Mar 19, 2024 •

edited

Loading

github-actions Bot commented Mar 19, 2024 •

edited

Loading