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

added propertyAlias parameter to resolveFile method (#2898 take 2) #3409

Closed
wants to merge 1 commit into from
Closed

added propertyAlias parameter to resolveFile method (#2898 take 2) #3409

wants to merge 1 commit into from

Conversation

skttl
Copy link
Contributor

@skttl skttl commented Oct 23, 2018

Prerequisites

Description

I added a propertyAlias parameter to the resolveFile method as suggested by @nul800sebastiaan in #2956 (comment).

To test

Add an upload (or image cropper) property to a media doctype. Try to link to the media, before resolveFile would return null, because it would hit the new upload property (being empty). After this, resolveFile is instructed to only look for the property called umbracoAlias, and returns the right url.

image

Gif before: https://i.imgur.com/AmbCcQn.gifv

Gif after: https://i.imgur.com/4QSgCYZ.gifv

Copy link
Member

@nul800sebastiaan nul800sebastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skttl Ouch, that's a lot of "umbracoFile" everywhere. I haven't tried this, but how about:

  1. There is a global const in C# for the umbracoFile alias, I'm sure you can access it in the JS as well, make sure to use that
  2. Instead of passing in "umbracoFile" everywhere, why not make it the default value if you don't pass in an alias? I am not sure if that would break existing usages, but I can't imagine it would as the propertyAlias parameter was never there before.

@skttl
Copy link
Contributor Author

skttl commented Oct 24, 2018

  1. I'll see if I can find it, any pointers would be appreciated :)

  2. I thought of adding umbracoFile as the default, but went the other way in case any thirdparties were using the method. For Umbraco there is no worries about having it as the default, as I changed all references to resolveFile to use that parameter.

@nul800sebastiaan
Copy link
Member

@skttl Ah! You shouldn't need to pass it in, it's already available on: mediaItem.metaData.umbracoFile.PropertyEditorAlias

I know what you mean about #2 but I would still try to see if we can have an option where we, for example, try to get the url and if it doesn't result in anything, try to get it from the umbracoFile property instead, if it exists.

Again, I haven't looked into it, just trying to not make it look messy ;-)

@skttl
Copy link
Contributor Author

skttl commented Oct 25, 2018

Are you sure about that regarding mediaItem.metaData.umbracoFile?

This is the result from GetChildren:
image

This is the mediaItem inside the method:
image

@skttl
Copy link
Contributor Author

skttl commented Oct 25, 2018

@nul800sebastiaan
I can make it find the value of umbracoFile as a fallback in case it is going to return an empty string. The problem about that is, if you have something in that other upload property. Then resolveFile would return the url to that file in stead. So we need to use the "look for umbracoFile approach" by default.

Or, we could make a new method called resolveUrl, which would look only for the value of umbracoFile

What do you think?

@zpqrtbnk zpqrtbnk changed the base branch from dev-v7 to v7/dev March 31, 2019 16:33
@nul800sebastiaan
Copy link
Member

Hey there @skttl !

I'm writing today after announcing at Codegarden, our yearly conference, that the upcoming version 7.15.0 will be the last Umbraco v7 release with new features in it. From now on all our efforts will be focused on version 8 instead. 🎱

We're wrapping up the 7.15 version at the moment and unfortunately these proposed changes won't be able to make it in there.

We'd like to extend a big thank you the efforts on this so far and we would love it if you could have a look at porting this feature over to v8 instead. Of course if you have any questions on where to start then feel free to ask on the forum, we monitor all questions there and we're happy to assist.

Thanks again for the work in this PR and we hope to see you contributing to v8 some time soon!

Best, Sebastiaan and the Umbraco CMS team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants