Skip to content
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

Conversation

Mizan3050
Copy link
Contributor

@Mizan3050 Mizan3050 commented Aug 30, 2023

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

@netlify
Copy link

netlify bot commented Aug 30, 2023

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit 5416160
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/64fc2e57f19a960008e5db83

@michaelbromley
Copy link
Member

Hi! Just wanted to let you know I saw this and will properly review later this week.

Copy link
Member

@michaelbromley michaelbromley left a 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[];
Copy link
Member

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);
Copy link
Member

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$;
Copy link
Member

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[];
Copy link
Member

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[];

@Mizan3050
Copy link
Contributor Author

Hi!!
I have updated the PR with requested changes

@michaelbromley michaelbromley merged commit bd834d0 into vendure-ecommerce:master Sep 11, 2023
15 checks passed
@michaelbromley
Copy link
Member

Thanks for your help on this! 🙏 🥳

michaelbromley pushed a commit that referenced this pull request Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🚀 Shipped
Development

Successfully merging this pull request may close these issues.

2 participants