fix(assets): Fix asset genration #3485
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix the generation of asset - the filter is incorrect and fails with `Cannot read property 'splashImages' of null, TypeError: Cannot read property 'splashImages' of null` Also fix the scale - it has been incorrectly declared as number, when it is string (1x, 2x, etc.). Use the scale correctly by parsing it and use it when generate the image. Also fix the using of resizeOperation property - it was not passed correctly for iOS assets.
Fatme
reviewed
Mar 22, 2018
| @@ -55,7 +56,14 @@ export class AssetsGenerationService implements IAssetsGenerationService { | |||
|
|
|||
| for (const assetItem of assetItems) { | |||
| const operation = assetItem.resizeOperation || Operations.Resize; | |||
| const scale = assetItem.scale || 0.8; | |||
| let tempScale: number = null; | |||
There was a problem hiding this comment.
In case when assetItem.scale is a number, scale will always be 0.8 and assetItem.scale will be disregarded. Is it the desired behaviour?
If not it should be let tempScale = assetItem.scale
There was a problem hiding this comment.
Thanks , I've fixed it in the code below
| image.height = assetItem.height; | ||
| image.size = `${assetItem.width}${AssetConstants.sizeDelimiter}${assetItem.height}`; | ||
| // Find the image size based on the hardcoded values in the image-definitions.json | ||
| _.each(imageDefinitions, (assetSubGroup: IAssetItem[]) => { |
There was a problem hiding this comment.
Just a suggestion (not a merge stopper), not sure if it is working correctly.
_(imageDefinitions)
.filter((assetSubGroup: IAssetItem[]) => _.find(assetSubGroup, assetElement => assetElement.filename === image.filename && path.basename(assetElement.directory) === path.basename(dirPath)))
.map((assetItem: IAssetItem) => {
if (image.size) {
// size is basically <width>x<height>
const [width, height] = image.size.toString().split(AssetConstants.sizeDelimiter);
if (width && height) {
image.width = +width;
image.height = +height;
}
}
if (!image.width || !image.height) {
image.width = assetItem.width;
image.height = assetItem.height;
image.size = image.size || `${assetItem.width}${AssetConstants.sizeDelimiter}${assetItem.height}`;
}
});
ivanovit
reviewed
Mar 22, 2018
| tempScale = splittedElements && splittedElements.length && splittedElements[0] && +splittedElements[0]; | ||
| } | ||
|
|
||
| const scale = tempScale || 0.8; |
There was a problem hiding this comment.
The scale must be applied to all resize operations.
Set correct sizes for iOS images in image-definitions.json. Also fix the code that resizes the images and overlay images. Add option to have overlayImageScale in the json. We've not set it now, but the code will respect it in case we decide to add it.
ivanovit
approved these changes
Mar 22, 2018
Fatme
reviewed
Mar 22, 2018
| } | ||
| } | ||
|
|
||
| const scale = tempScale || 1; |
There was a problem hiding this comment.
1 can be extracted as a constant.
Fatme
reviewed
Mar 22, 2018
|
|
||
| switch (operation) { | ||
| case Operations.OverlayWith: | ||
| const imageResize = Math.round(Math.min(assetItem.width, assetItem.height) * scale); | ||
| const overlayImageScale = assetItem.overlayImageScale || 0.8; |
There was a problem hiding this comment.
Also 0.8 can be extracted as a constant
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the generation of asset - the filter is incorrect and fails with
Cannot read property 'splashImages' of null, TypeError: Cannot read property 'splashImages' of nullAlso fix the scale - it has been incorrectly declared as number, when it is string (1x, 2x, etc.). Use the scale correctly by parsing it and use it when generate the image.
Also fix the using of resizeOperation property - it was not passed correctly for iOS assets.
chore: Update to latest common lib