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

Implement metadata image selection #1058

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Jetroid
Copy link

commented Nov 13, 2017

@dereklieu: Implements issue #998 without using an external service. #998 is an important issue for me and I cannot use prose without this feature.

Usage:

prose:
  metadata:
    _posts:
      - name: "image"
        field:
          element: "image"
          placeholder: "default-image.jpg"
          label: "My Image"

Possible extension functionality:

Add a _prose.yml config value to allow configuration of it metadata is set equal to the filename, eg background-homepage.png, directory, eg /assets/images/background-homepage.png or fully qualified, eg {{ site.baseurl }}/assets/images/background-homepage.png.

@dereklieu

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

Hey @Jetroid thanks for pulling in the time here. Just wanted to say that this is on my radar, even though I haven't looked at it yet, I plan to over the next day or two.

@ewiggin

This comment has been minimized.

Copy link

commented Feb 9, 2018

@dereklieu I'm waiting for this feature. 😄 What is the state of this?

@dereklieu

This comment has been minimized.

Copy link
Member

commented Feb 9, 2018

My bad! Really blocking this but was buried under work for the past few months. I have some more time though and promise to look at this over the weekend.

app/util.js Outdated
@@ -257,6 +257,42 @@ module.exports = {
return false;
},

renderMedia: function(data, elem, controller, back) {

This comment has been minimized.

Copy link
@dereklieu

dereklieu Feb 12, 2018

Member

I know this function is now repurposed to be called from two places, so I'm glad you put it in a util. However, I don't love the idea of passing around references to jQuery objects. Between different files, it gets a lot harder to know what's being referenced. Is there some way we can avoid this?

app/util.js Outdated
@@ -257,6 +257,42 @@ module.exports = {
return false;
},

renderMedia: function(data, elem, controller, back) {

This comment has been minimized.

Copy link
@dereklieu

dereklieu Feb 12, 2018

Member

Since this is now in a different file, maybe data should be more descriptive.

this.updateImageInsert(e, file, content);
var $dialog = this.$el.find('div[data-element="dialog"]');
if ($dialog.hasClass('dialog')) {
this.updateImageInsert(e, file, content, this.toolbar, dialog);

This comment has been minimized.

Copy link
@dereklieu

dereklieu Feb 12, 2018

Member

Seems like the argument should be $dialog and not dialog.

var src = path + '/' + encodeURIComponent(file.name);
updateImageInsert: function(e, file, content, target, $dialog) {
var path = (this.config.media) ?
this.config.media :

This comment has been minimized.

Copy link
@dereklieu
this.updateSaveState(message, 'error');
}).bind(this)
success: (successCb).bind(this),
error: (failureCb).bind(this)

This comment has been minimized.

Copy link
@dereklieu

dereklieu Feb 12, 2018

Member

The error callbacks are all the same I think, so that can probably just stay put?

if (this.queue) {
var userDefinedPath = $('input[name="url"]').val();

var onSuccess = function(model, res, options) {

This comment has been minimized.

Copy link
@dereklieu

dereklieu Feb 12, 2018

Member

I would guess that this is where it's failing to set the save state, and causing the spinner to endlessly spin.

@dereklieu

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

@Jetroid first off, sorry for my tardiness here. You've done some great work in a codebase that isn't exactly easy to work with (and is downright nasty by today's standards) so 👍 for that.

I left some notes here. Overall I think this is on the right path but I think there are some changes I'd like to see before merging this. The main thing not mentioned here is unit tests. I wonder if breaking out some of this functionality that's now in the file, toolbar, and metadata views could be better served living in a separate, new image uploader view? I think that might help avoid some of the $.closest calls, which I'm not terribly excited about. This would also help improve testability, which would probably be a real chore the way it is right now, though that is certainly not your fault.

Happy to hear your thoughts here and help out however I can. Thanks again.

@kylekirkby

This comment has been minimized.

Copy link

commented Jan 3, 2019

Hi all,

Any chance on getting this feature merged ?

Best regards,
Kyle

@Jetroid Jetroid force-pushed the Jetroid:master branch from 0956aa7 to 29b0f35 Jan 27, 2019

@Jetroid

This comment has been minimized.

Copy link
Author

commented Jan 27, 2019

Hi @dereklieu, sorry, I completely forgot about Prose and this pull request until Kyle posted his reminder.

I've taken another stab at this, and have broken out a lot of the functionality into a new view, AssetSelection, which renders the contents of the dialog when picking/uploading an image.

Hopefully this is more in line with the sort of thing you want for Prose.

@kylekirkby

This comment has been minimized.

Copy link

commented Jan 28, 2019

LGTM - thanks @Jetroid !

@neosilky

This comment has been minimized.

Copy link

commented Jan 31, 2019

Would love this! 😄

@pnochisaki

This comment has been minimized.

Copy link

commented Jan 31, 2019

Me three

@Jetroid

This comment has been minimized.

Copy link
Author

commented Feb 20, 2019

Hey @dereklieu, I'd like to avoid us forgetting about this again, if possible!

@Jetroid

This comment has been minimized.

Copy link
Author

commented May 1, 2019

Ping @dereklieu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.