Feature/replace and find image#236
Conversation
…ping + testing edging cases
|
This PR is so relevant, why it seems abandoned by over 7 months now? |
pedrobonamin
left a comment
pedrobonamin
left a comment
There was a problem hiding this comment.
Hello @Polleke007 thank you for your contribution.
It looks great so far, thank you very much for building this, I'm sorry it took us so long to reply here, I have added some comments so we can move this forward.
It would be amazing if we can move this into the assets detail dialog, that way we have a place to show the documents that are referencing this asset and that will be updated, also I would love to have a loading state in the reference docs while they are being updated this way users can understand what change is taking place and when it will be done and a confirm dialog before taking action will also be highly appreciated, this could be quite destructive if someone runs the replacement on hundred of docs and then they need to revert it.
Another thing that will be worth having is moving this feature behind a config option in the plugin, some users could need to opt out from this due to the potential to update many documents at once.
We are updating this repo I have added tests and we eventually want to move it to the plugins monorepo, where it will be easier to collaborate and it will be easier for us from Sanity to test and validate changes.
Would you also mind updating this branch with the latest changes introduced in main ? there are some conflicts on the PR
| type Props = { | ||
| id: string | ||
| selected: boolean | ||
| source?: string |
There was a problem hiding this comment.
Could we type this as source?: 'replace-asset' using this will make it simpler for anyone coming later to understand which values this string could have.
| ), | ||
| updateImageReferences: createAction( | ||
| 'actions/updateImageReferences', | ||
| function prepare({assets, id}: {assets: AssetItem[]; id: String}) { |
There was a problem hiding this comment.
| function prepare({assets, id}: {assets: AssetItem[]; id: String}) { | |
| function prepare({assets, id}: {assets: AssetItem[]; id: string}) { |
| for (const assetToReplace of assetsToReplace) { | ||
| await client | ||
| .patch(document._id) | ||
| .set(assetToReplace as AttributeSet) | ||
| .commit() |
There was a problem hiding this comment.
Could this be a unique transaction per document instead in where we replace all the assets in the file?
Also, could we add .ifRevisionId(document._rev) to the patch? That way if the document changed since we did this we get a safer change
| @@ -0,0 +1,44 @@ | |||
| import {Asset} from '@types' | |||
There was a problem hiding this comment.
This needs to be from ../types right?
| import {Asset} from '@types' | ||
|
|
||
| export function findImageAssets<T extends {_id: string}>( | ||
| document: T, |
There was a problem hiding this comment.
Should this instead be SanityDocument?
| document: T, | |
| import type {SanityDocument} from '@sanity/client' | |
| document: SanityDocument, |
| import {Box} from '@sanity/ui' | ||
| import React, {ReactNode} from 'react' | ||
| import Dialog from '../Dialog' | ||
| import {DialogAllAssetsProps} from '@types' |
There was a problem hiding this comment.
This needs to be relative
| import {DialogAllAssetsProps} from '@types' | |
| import {DialogAllAssetsProps} from '../../types' |
| const assetsToReplace = findImageAssets(clonedDocument, asset, id) | ||
| for (const assetToReplace of assetsToReplace) { | ||
| await client | ||
| .patch(document._id) |
There was a problem hiding this comment.
This could be potentially harmful and affect multiple documents and even published document, we tend to not update published documents from actions taken in the studio, you can update published only by publishing.
What do you think if we make this patch the draftId of the document instead?
We will need to first filter and and check if a existing draft exists for that document or not, if it doesn't exists we need to create it.
If it exists, the published document here needs to be dropped, to avoid updating it and we run the update through the draft instead.
Also, I think it will be very beneficial if we can show a list of the documents that will be updated. So users know what changes will be happening when doing this update.
| {assetsPicked.length === 1 && ( | ||
| <Button | ||
| mode="bleed" | ||
| onClick={handleReplaceImages} | ||
| padding={2} | ||
| style={{background: 'none', boxShadow: 'none'}} | ||
| tone="default" | ||
| > | ||
| <Label size={0}>Replace</Label> | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
I think this feature makes sense to be part of the assets detail dialog.
This bar is for bulk actions, like deleting, which can happen in multiple documents.
If we do it from the dialog we can take advantage of the existing references tab, this tab will give the users a view of which items they will be updating by running this action
What do you think if we add it here?
| mergeMap(() => client.fetch(`*[references("${id}")]`)), | ||
| mergeMap(async documents => { | ||
| for (const document of documents) { | ||
| const clonedDocument = JSON.parse(JSON.stringify(document)) |
There was a problem hiding this comment.
Do we need here to clone the document? I think we can pass it directly to the function as it is received from the fetch
| @@ -0,0 +1,44 @@ | |||
| import {Asset} from '@types' | |||
There was a problem hiding this comment.
Can we make this file lowerCase? Also better if we can rename it to findImageAssets
3 participants
Feature request for #166