-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Output largo specific photo credit metadata on image block #1733
Output largo specific photo credit metadata on image block #1733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
…etadata-on-image-block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PullRequest network review has been cancelled
You can reactivate the code review job from the PullRequest dashboard - or - by adding [pr] to the title of this code review.
Questions for review:
|
|
On the first save of a post containing a newly-inserted image block for an image that contains a media credit, the The media credit doesn't capture the organization field from the media credit metadata, in my testing. |
But all that criticism aside: This is a massive improvement over the status quo, and it proves that the thing we're trying to do is, in theory, possible. 🎉 |
@benlk Do you think it's worth hacking our way through this right now instead of using something like |
If there's no way to make it work on the |
Coming in Gutenberg 6.1: https://developer.wordpress.org/block-editor/components/slot-fill/ |
…etadata-on-image-block
@benlk Looking more into this here WordPress/gutenberg#10204 brings up a good point. If we continue down the path of manipulating the image block and then someone chooses to switch off of Largo, all of their image blocks (presumably) will go into the "Invalid" state since it won't match its expected output. |
I'm not sure that we need to be worried about that, since we're not adding content that is invalid in the area that we're adding it. The examples in WordPress/gutenberg#10204 show adding new I'm currently running the #1662 branch of Largo on a database where I had previously used this branch to test the addition of elements. This gives us the setup: where the code in this PR had been active, but is not. Here's the big conflict: Parts:
|
Thanks for looking at that so quickly. |
media credit, and also use correct class and structure
@benlk I think this is ready for review again. I addressed all of your comments. I also switched from using It also seems to be backwards compatible now and doesn't break blocks when you switch to a different branch without this code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs support for the media credit URL, but apart from that I think this is good to go. 👏
js/blocks-image-customization.js
Outdated
// our default media credit, if it exists | ||
var media_credit = '<p class="wp-media-credit">' + media.media_credit._media_credit + '</p>'; | ||
|
||
// if our media credit has a url, include that also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section tests if it has a media credit org, not if it has a URL.
This PR does not, presently, include a function for including the URL in the media credit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benlk I wasn't sure about the URL since it was set as a variable but wasn't used in the classic editor way here:
https://github.com/INN/largo/blob/39a0fc4ca34a118ae6ae870dd591029f5a61fdb1/lib/navis-media-credit/navis-media-credit.php#L248-L250
Should the URL wrap the credit or organization name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd that it isn't used in the main media credit instance.
It's used for the "hero" featured image on posts, where the link wraps the credit and org name together: https://github.com/INN/largo/blob/72cc8433a1be1de4f22789e1ec89496d54a653d9/partials/hero-featured-image.php#L5-L7
This PR doesn't modify the Gallery Block, but if we want to cover that I think it should be in a separate PR, for a separate issue. |
has/doesn't of:
But the case where an image has a URL alone makes little sense. So let's add:
|
@benlk I'll have to rewrite part of this if we want these two to work:
Right now the first conditional just checks if the credit exists and doesn't show anything if the credit doesn't exist.
|
5ffccaf updates the conditional to allow the organization (+url if it exists) to display if the credit doesn't exist. |
If that worked in your testing, let's merge it and be done. |
Changes
This pull request makes the following changes:
Why
For #1683.
Testing/Questions
Features that this PR affects:
Questions that need to be answered before merging:
Steps to test this PR:
a. One image with a media credit with no url
b. One image with a media credit with a url