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

Move Featured Media UI to Featured Image Metabox #1285

Merged
merged 10 commits into from
Sep 22, 2016

Conversation

rclations
Copy link

fixes #698

The Problem

  • Featured image override checkbox is hidden at the bottom of the featured image selection modal
  • The wording on the checkbox is confusing
  • Options in the image selection modal should apply to that specific image - this checkbox is applies to the post (not the image itself)
  • The existing "Set Featured Media" button is located above the wysiwyg editor. This area is generally used for shortcode buttons to be used within the wysiwyg editor.

Proposed Solution

The featured media metabox was removed so we could build our own button with additional options for featured video, embed, gallery, etc.

This PR adds a new featured media metabox and moves the "Set Featured Media" button there.

@aschweigert
Copy link

@rclations same issue here :/ I'm not able to check out this branch to review because it's on your fork. Can you move that branch over here so I can do some final review/cleanup before merging?

@rclations
Copy link
Author

@aschweigert can you access it now? I just checked the new "allow edits from maintainers" feature

also add some missing translation strings
@aschweigert
Copy link

aschweigert commented Sep 21, 2016

ok, this lgtm, a couple to-do items before merging:

  • need to make the featured media templates translatable (currently nothing in there is)
  • consider refactoring the duplicative featured media functions (there's a set for each type of featured media that do nearly the same thing, let's try to see if we can refactor that and make it a bit cleaner)

@rclations
Copy link
Author

should be all set, but @aschweigert or @benlk, could you give this a quick review before I merge?

@benlk
Copy link
Collaborator

benlk commented Sep 22, 2016

I'm getting a featured-media.min.js?ver=4.6.1:1 Uncaught TypeError: Cannot read property 'replace' of undefined when running with LARGO_DEBUG set to False, but not when True.

With True, it works pretty well, but should probably be higher in the stack on the page:

screen shot 2016-09-22 at 4 58 12 pm

screen shot 2016-09-22 at 4 58 21 pm

screen shot 2016-09-22 at 4 58 28 pm

screen shot 2016-09-22 at 4 58 34 pm

@aschweigert
Copy link

@jackbrighton we'll need docs coverage for this since we're moving peoples' button. Let's make sure that gets done before we ship 0.5.5

Copy link
Collaborator

@benlk benlk left a comment

Choose a reason for hiding this comment

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

Found source of bugs in #1322 with non-image featured media.

return;
switch( $featured_media['type'] ) {
case 'video':
$template_slug = 'video';
Copy link
Collaborator

Choose a reason for hiding this comment

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

partials/hero-featured-video is not a file that exists.

This line should use $template_slug = 'embed', to match the old code:

if (in_array($featured_media['type'], array('embed-code', 'video'))) {
    $ret = largo_get_featured_embed_hero($post->ID,$classes);

@@ -157,109 +132,51 @@ function largo_get_featured_image_hero($post = null, $classes = '') {
'the_post' => $the_post
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're adding partials/hero-featured-embed.php to the list of things that get rendered with this context, then $featured_media and $gallery_ids need to be passed in this array if that's appropriate to the rendering.

@benlk
Copy link
Collaborator

benlk commented Oct 3, 2016

The code changes in this PR included a lot of things that weren't mentioned in the pull request's description. ☹️

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.

None yet

3 participants