Fix: Copy Path does not reflect the real path of the object in the media folder #5571
Conversation
see the issue for details fixes issue decaporg#5569
There was a problem hiding this comment.
Hi @DenysLins! Thanks for opening the PR.
Some comments:
- Should we only prepend the
/if the configmedia_folderhas a forward/? With this PR/is always added. pathcan be an absolute URL (not very documented, but possible with the asset store integration). Should we only prepend if the path is not an absolute URL?
add a condition to verify if the path already starts with /, so we must not add another one fixes issue decaporg#5569
add a condition to only add a / is the path is not absolute fixes issue decaporg#5569
There was a problem hiding this comment.
Thanks for the quick follow up @DenysLins.
See my additional comment
| const { draft, name } = this.props; | ||
| let path = this.props.path; | ||
| if (isAbsolutePath(path)) { | ||
| copyToClipboard(path); |
| if (isAbsolutePath(path)) { | ||
| copyToClipboard(path); | ||
| } else { | ||
| path = path.charAt(0) !== '/' ? `/${path}` : path; |
There was a problem hiding this comment.
Hi @DenysLins, I think I wasn't clear with my last comment.
- If
media_folder: static/upload-> The copied path should not start with a/ - If
media_folder: /static/upload-> The copied path should start with a/
It seems to me that this code prepends a / if it doesn't already exist. Please correct me if I missed anything.
There was a problem hiding this comment.
I got it. I will work in this issue...
pass the media_folder parameter and use it before setting the path fixes issue decaporg#5569
Now media_folder must starts with / and path must not starts to be updated fixes issue decaporg#5569
There was a problem hiding this comment.
Thanks @DenysLins, I added another comment as I believe we're missing some use cases.
| /> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| function mapStateToProps(state) { | ||
| const { mediaLibrary } = state; | ||
| const mediaFolder = state.config.media_folder; |
There was a problem hiding this comment.
Thanks @DenysLins, this might not be accurate as media folders can be configured per collection and per field.
We might need to pass the untrimmed value from this function https://github.com/netlify/netlify-cms/blob/b94d5f6e7fe07a2816098b377b30a4ddd39c9520/packages/netlify-cms-core/src/reducers/entries.ts#L754
There was a problem hiding this comment.
OK @erezrokah ... I will apply your suggestions...
|
Closing this as stale. Please comment if you'd like to pick this up |
|
From an end user's perspective if the "public_folder" vs the "media_folder" suffix was used in the "Copy path" function this would match usage for wanting to link to a PDF or non image media item using the "link" button from the markdown editor. |
|
It's been a long time but problem is still there, could we do something for it ? |
|
@pierreleripoll I can reopen this PR if you want to work on it, let me know. |
fixes #5569
Summary
In the media assets screen, there is a useful Copy Path button. When the user clicks on the button, the path copied does not reflect the path that is specified in the backend. For example, it starts with uploads when it should start with /uploads.
Test plan
A picture of a cute animal (not mandatory but encouraged)