fix: image asset ids #107
fix: image asset ids #107
Conversation
🦋 Changeset detectedLatest commit: 1bc5558 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
There was a problem hiding this comment.
I think we need to establish a clear distinction between the following in the LottieImage class:
idfileName
In this update, we're relying on fileName as the new identifier, so we might no longer need the id property for images.
or, we could continue using id as the filename, inferring the extension automatically from the image data ?
|
|
||
| this.animations.forEach((animation) => { | ||
| images.set(animation.id, animation.imageAssets); | ||
| animation.imageAssets.forEach((imageAsset) => { | ||
| if (dupeMap.has(imageAsset.id)) { |
There was a problem hiding this comment.
The id is not always the same as the filename, for example
id: image_0, filename: image_0.png
is not the same as
id: image_0, filename: 'image_0.webp'
right ? probably our ids should be the filename and we can get rid of using the image id in this case, no ?
There was a problem hiding this comment.
Inside the lottie itself, images had an id field, and then a name and path attribute, thats why I mapped it like this to the image class.
The id is just the filename without the extension
In your example the id stayed the same despite changing the extension
| * @returns Map of duplicate image ids and their count. | ||
| */ | ||
| private _getMapOfDuplicateImageIds(): Map<string, number> { | ||
| let count = 0; |
There was a problem hiding this comment.
Why do we need this global count variable ? isn't the map already has the occurrence of each image by its ID ?
| count = dupeMap.get(imageAsset.id) ?? 0; | ||
| dupeMap.set(imageAsset.id, count + 1); | ||
| } else { | ||
| dupeMap.set(imageAsset.id, 1); |
There was a problem hiding this comment.
This seems to be an occuranceMap not duplicateMap, right ? as based on this line, even if there is a unique image id, still would be pushed to the map with an occurance of 1
There was a problem hiding this comment.
It was duplicateMap as duplicate filename not the content of the image, will change to filename occurence map
| @@ -140,6 +140,8 @@ export class LottieImageCommon { | |||
| } | |||
| // Default to png if the file extension isn't available | |||
| this.fileName = `${newName}.${fileExt}`; | |||
| } else { | |||
| this.fileName = `${newName}.png`; | |||
There was a problem hiding this comment.
Isn't the extension inferred from the mimetype of the data ? defaulting it to png is safe ?
No description provided.