feat(lib): provide more properties to selected asset #395
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
eunjae-lee
changed the title
| focus: '', | ||
| alt: '', | ||
| source: '', | ||
| is_private: false, |
There was a problem hiding this comment.
Hey @eunjae-lee 👋
I don't know how feasible it would be but would be possible to also have the is_external_url property added? 🤔
May it not be possible or not make sense but just commenting once I saw this property exists in the image shared on the issue's description.
There was a problem hiding this comment.
Hmm 🤔 I actually don't know about external asset. Do you know @Dawntraoz maybe? I want to test a situation where is_external_url is true.
There was a problem hiding this comment.
Indeed @eunjae-lee, you need to go to the image field definition in the Block and check the Allow External URL, then when creating the story, instead of selecting an asset from the Assets Library, you should click on the externalURL icon and add a link to an image (for example for this one: https://letsenhance.io/static/8f5e523ee6b2479e26ecc91b9c25261e/1015f/MainAfter.jpg). Then you will be able to see the is_external_url to true!
There was a problem hiding this comment.
Thank you @Dawntraoz! I've followed your instruction, and I was able to upload an external asset.
But, in asset selector, I don't see that. I feel like this should be normal, right? (It's just attached to the story as an external URL, but not really uploaded to my space).
So, if I understand this correctly, the selected asset can never be external. What do you think?
There was a problem hiding this comment.
You have a point since the message sent only involves Asset Manager iteration in the field plugin. It will always be false 🤔
There was a problem hiding this comment.
I guess that's why is_external_url even doesn't exist on storyfront right before it's passed down to field plugin.
There was a problem hiding this comment.
Nice improvement, @eunjae-lee 👏
Left a question but overall, great work 🚀
| @@ -231,9 +231,29 @@ describe('createPluginActions', () => { | |||
| field: 'dummy', | |||
| callbackId: TEST_CALLBACK_ID, | |||
| filename, | |||
| fieldtype: 'asset', | |||
| name: '', | |||
| meta_data: {}, | |||
There was a problem hiding this comment.
Just a question: Since metadata has already title, source, and such, why do we need the properties twice?
There was a problem hiding this comment.
I'm not 100% sure, but I think it can contain more than that.
https://www.storyblok.com/docs/api/management/core-resources/assets/the-asset-object
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
so quick, love this! i think once its done we can even share this on discord and reply to the specific threads/feature requests where people requested this (if we find them)
I would wait for the changes that were applied on storyfront and then approve so that we do not accidentally approve before :)
🚀
| @@ -1,22 +1,37 @@ | |||
| import { hasKey } from '../../../utils' | |||
| import { AssetSelectedMessage } from './AssetSelectedMessage' | |||
|
|
|||
| export type AssetWrapper = { | |||
There was a problem hiding this comment.
Previous Asset has been renamed to AssetWrapper, and AssetWrapper['asset'] is now called Asset that is a full object for the asset.
Storyfront sends AssetWrapper to field plugins, and field plugin unwrapped it and return its Asset to field plugin users.
There was a problem hiding this comment.
| @@ -231,9 +231,33 @@ describe('createPluginActions', () => { | |||
| field: 'dummy', | |||
| callbackId: TEST_CALLBACK_ID, | |||
| filename, | |||
| asset: { | |||
There was a problem hiding this comment.
I know these are just tests, but maybe it's useful to create an asset variable to share among them to avoid duplicating the fields everywhere.
export const emptyAsset: {
id: 0,
fieldtype: 'asset',
name: '',
filename: '',
meta_data: {},
title: '',
copyright: '',
focus: '',
alt: '',
source: '',
is_private: false,
}
| @@ -35,24 +56,8 @@ describe('Asset', function () { | |||
| assetFromAssetSelectedMessage(assetSelectedMessage), | |||
| ).toHaveProperty('filename') | |||
| }) | |||
| it('keeps unknown properties', () => { | |||
There was a problem hiding this comment.
This was intended to allow properties that might be added on the storyfront side, but now that we're wrapping all the "potential" additions under "asset" property, we no longer need this.
| filename: 'https://somthing.com/myimage.jpg', | ||
| }), | ||
| ).toEqual(true) | ||
| const { field: _field, ...withoutField } = stub |
There was a problem hiding this comment.
fixing flaky test
eunjae-lee
dismissed
BibiSebi’s stale review
The corresponding PR on storyfront has been just merged :)
What?
This PR should be released only after https://github.com/storyblok/storyfront/pull/6357 is released.
Why?
JIRA: SHAPE-7043
fixes: #392
How to test? (optional)
I've confirmed working (local storyfront + locally built field plugin pointing to local storyfront as postMessage origin)
