-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add image variants viewer #136
Conversation
446303c
to
39d4a4b
Compare
39d4a4b
to
adbec9a
Compare
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.
Awesome change, thx!
Left some questions 🙂
public function assetVariants($_, array $variables, AssetSourceContext $assetSourceContext): array | ||
{ | ||
$assetProxy = $this->asset($_, $variables, $assetSourceContext); | ||
if ($assetProxy === null || !($assetProxy instanceof NeosAssetProxy) || !($assetProxy->getAsset() instanceof VariantSupportInterface)) { |
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.
Do we need the check here for the NeosAssetProxy
if we already check for the interface?
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.
i don't really know, I took that from https://github.com/neos/neos-development-collection/blob/master/Neos.Media.Browser/Classes/Controller/AssetController.php#L421 and it made sense to me to check if assetProxy
is a NeosAssetProxy
and so has the method getAsset
, before calling it. But i am not proficient enough in the neos asset handling to judge if it is really necessary
...ces/Private/JavaScript/media-module/src/components/SideBarRight/Inspector/AssetInspector.tsx
Outdated
Show resolved
Hide resolved
Resources/Private/JavaScript/media-module/src/components/Presentation/Column.tsx
Show resolved
Hide resolved
Should we then call this PR variant viewer for now and add the editor in a separate PR? |
Thx for the changes, should I then make the final review and we can merge if it's fine? |
Sounds good to me, I have plans to add the editing feature this week, but that depends on my workload, no need to block this PR |
This PR adds the possibility to view image variants as a foundation to implement the variant editing as well.
To test the feature, you need to configure image variant presets as described here.
You also need to enable the variants editor by setting