-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
slide of image (carousel) added in the assets dialog admin-ui panel #2370
slide of image (carousel) added in the assets dialog admin-ui panel #2370
Conversation
✅ Deploy Preview for effervescent-donut-4977b2 canceled.
|
Hi! Just wanted to let you know I saw this and will properly review later this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this is a great PR, thanks so much. It really improves the UX!
I have a few small issues to fix, and then can you also make the PR against the minor
branch rather than master
, since it is a new feature and will go in the next minor release v2.1
asset: AssetLike; | ||
assets: AssetLike[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's mark this as optional in case anyone already uses this component before this change, they won't be passing in the locals: { assets }
array.
@@ -46,6 +48,8 @@ export class AssetPreviewDialogComponent implements Dialog<void>, OnInit { | |||
} | |||
}), | |||
); | |||
|
|||
this.assetsWithTags$ = of(this.assets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise this would then be changed to
of(this.assets ?? [])
assetChanges?: UpdateAssetInput; | ||
resolveWith: (result?: void) => void; | ||
assetWithTags$: Observable<GetAssetQuery['asset']>; | ||
assetsWithTags$; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a type:
assetsWithTags$: Observable<Array<GetAssetQuery['asset']>>;
@@ -33,6 +33,7 @@ export type PreviewPreset = 'tiny' | 'thumb' | 'small' | 'medium' | 'large' | '' | |||
}) | |||
export class AssetPreviewComponent implements OnInit, OnDestroy { | |||
@Input() asset: AssetLike; | |||
@Input() assets: AssetLike[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mark this as optional since existing external uses of the component will not be passing it.
@Input() assets?: AssetLike[];
Hi!! |
Thanks for your help on this! 🙏 🥳 |
implemented a generalized solution where we have a number of assets which can be opened in the dialog from:
product detail view
product variant detail view
collection detail view
asset list view