Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23198 closed enhancement (fixed)

Pass post format as a class to TinyMCE init

Reported by: helen's profile helen Owned by: azaozz's profile azaozz
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Post Formats Keywords: has-patch
Focuses: Cc:

Description

Previously: #19437 for post type.

Attachments (8)

23198.patch (555 bytes) - added by azaozz 11 years ago.
23198-2.patch (781 bytes) - added by azaozz 11 years ago.
23198-3.diff (1.7 KB) - added by adamsilverstein 11 years ago.
add-WP_Error.diff (440 bytes) - added by adamsilverstein 11 years ago.
adds WP_Error return to get_post_format
23198-4.diff (2.4 KB) - added by adamsilverstein 11 years ago.
cleared up phpdoc
23198.diff (2.2 KB) - added by wonderboymusic 11 years ago.
23198.2.diff (2.6 KB) - added by adamsilverstein 11 years ago.
updated tinymce container class dynamically when changing post-format
23198.3.diff (760 bytes) - added by adamsilverstein 11 years ago.
cleanup as per suggestions

Download all attachments as: .zip

Change History (47)

#1 @sabreuse
11 years ago

  • Cc sabreuse added

#2 @adamsilverstein
11 years ago

  • Cc ADAMSILVERSTEIN@… added

#3 @WraithKenny
11 years ago

  • Cc Ken@… added

#4 @alex-ye
11 years ago

  • Cc nashwan.doaqan@… added

@azaozz
11 years ago

#5 @azaozz
11 years ago

Does this need anything more than 23198.patch?

#6 @helen
11 years ago

Well, needs to be format, not status :)

#7 @azaozz
11 years ago

Argh, was thinking it would be good to have the post_status as a class there too... Then forgot to add the main bit, post format :)

@azaozz
11 years ago

#8 follow-up: @adamsilverstein
11 years ago

my patch adds a little more error checking and sanitizing, and a post-standard-format fallback; i got the code basis from post-template.php; while there i also added some missing whitespace, sorry if that doesn't belong in this patch.

#9 @SergeyBiryukov
11 years ago

  • Keywords has-patch added; needs-patch removed

#10 in reply to: ↑ 8 @azaozz
11 years ago

Replying to adamsilverstein:

my patch adds a little more error checking and sanitizing, and a post-standard-format fallback;

As far as I see get_post_format() does not return WP_Error (phpdoc is wrong there?) and already checks whether the post type supports formats. Perhaps we should update get_body_class() in post-template.php.

#11 @adamsilverstein
11 years ago

i'm note sure if there is an advantage in returning WP_Error vs. returning false, maybe providing more information;

set_post_format does return WP_Error, maybe we should add these lines into get_post_format?

if ( empty($post) )

return new WP_Error('invalid_post', ('Invalid post'));


@adamsilverstein
11 years ago

adds WP_Error return to get_post_format

#12 follow-up: @adamsilverstein
11 years ago

add-WP_Error.diff adds the WP_Error return into get_post_format; I don;t really know if we need that check, or if we don't why we would want it in set_post_format; and the phpdoc needs updating.

#13 in reply to: ↑ 12 @azaozz
11 years ago

Replying to adamsilverstein:

Seems get_post_format() is called after we have the post, i.e. without passing an argument or passing $post->ID. However passing a non-existing post ID will throw a PHP notice. Not sure whether we need WP_Error there or simply:

if ( empty($post) )
    return $post;

This would need a new ticket in any case.

Last edited 11 years ago by azaozz (previous) (diff)

@adamsilverstein
11 years ago

cleared up phpdoc

#14 @adamsilverstein
11 years ago

updated phpdoc - $post is optional, since get_post_format calls get_post immediately, get_post_format will work with a post id, a post object or no post at all (uses the current post)

#15 @adamsilverstein
11 years ago

reviewing the code, i don;t see how the WP_Error is helpful, but maybe its a forward thinking bit of code. curious to see what others think.


#16 @rachelbaker
11 years ago

  • Cc rachelbaker added

#17 @azaozz
11 years ago

Initially it was returning WP_Error [15777], which was changed in [16319] together with changing wp_get_object_terms() to get_the_terms().

#18 follow-up: @adamsilverstein
11 years ago

so we can remove the check for WP_Error here, and then also remove that check from post-template.php?

#19 follow-up: @WraithKenny
11 years ago

What about when the post format is changed while editing the post? Will the tinymce class be updated?

Also, are there classes that get added to body_class or post_class that should be added? Should we rather populate the init with post_class()?

#20 @adamsilverstein
11 years ago

we should probably use get_post_class, not sure why that wasn't used originally, that would ensure all the same classes were passed to tinymce as get passed in other spots like actual post display on site. i'll redo it that way. as for updating when you change the post format, yes, it will change, once you you click update, not before then - would you expect/want it to, and if so, why?

#21 in reply to: ↑ 18 ; follow-up: @azaozz
11 years ago

Replying to adamsilverstein:

Opened #23255 for get_post_format(). Could you add your patches there.

#22 in reply to: ↑ 19 @azaozz
11 years ago

What about when the post format is changed while editing the post? Will the tinymce class be updated?

Good question. In the spirit of WYSIWYG-ness thinking we should change the body class in the Visual editor dynamically when the post format is changed.

Also, are there classes that get added to body_class or post_class that should be added? Should we rather populate the init with post_class()?

Not sure it's a good idea to add everything from post_class() to the body in TinyMCE. It is intended for the front-end and not intended for the body tag, so risking to bring compatibility problems. Also themes may not use post_class(). More advanced themes can add a body class to the editor with the init options for TinyMCE.

#23 follow-up: @helen
11 years ago

This is related to post formats UI, so I imagine that dynamically adding/changing would be ideal in order to have a chance at doing a truer WYSIWYG preview. Adding the rest of post_class() is likely unnecessary and introduces even more problems with dynamic changes (sticky, category, etc.). Also not in the scope of this ticket :)

#24 in reply to: ↑ 21 @adamsilverstein
11 years ago

Replying to azaozz:

Replying to adamsilverstein:

Opened #23255 for get_post_format(). Could you add your patches there.

ok, i can do that.

#25 in reply to: ↑ 23 ; follow-up: @adamsilverstein
11 years ago

Replying to helen:

This is related to post formats UI, so I imagine that dynamically adding/changing would be ideal in order to have a chance at doing a truer WYSIWYG preview. Adding the rest of post_class() is likely unnecessary and introduces even more problems with dynamic changes (sticky, category, etc.). Also not in the scope of this ticket :)

good points! i will try to cook up some jQuery to update the applied classes, fun!

#26 in reply to: ↑ 25 @azaozz
11 years ago

i will try to cook up some jQuery to update the applied classes, fun!

You will need to hook that up to TinyMCE. Something like:

if ( typeof tinymce != 'undefined ) {
  // get the new class
  jQuery('#post-formats-select input.post-format').on('change', function(){
    var ed = tinymce.get('content'), edBody, format;
  
    if ( ! ed )
      return;

    if ( this.id ) {
      edBody = ed.getBody();
      format = this.id; // also need to handle 'post-format-0'

      edBody.className = edBody.className.replace(/\bpost-format-[^ ]+/, '');
      ed.dom.addClass(edBody, format);
    }
  });
}
Last edited 11 years ago by azaozz (previous) (diff)

#27 @adamsilverstein
11 years ago

thanks, thats super helpful, seems like it... i'll try to insert that and give it a try. i was just going to traverse the dom down into the iframe but this makes much more sense.

question:

should post-format-0 even be there, that seems like a bug, shouldn't the 1st radio id be post-format-standard?

should i try to fix that or is there for legacy support of some kind, and if so should i catch that and rewrite in the js so the applied class is post-format-standard? i what will get_post_class() return?

(this - )
<input id="post-format-standard" class="post-format" type="radio" checked="checked" value="standard" name="post_format">
instead of
<input id="post-format-0" class="post-format" type="radio" checked="checked" value="0" name="post_format">

#28 follow-up: @azaozz
11 years ago

The comment by @helen got me thinking: we may want to dynamically change/replace a TinyMCE body class for something else in the future. We can abstract this bit and add it to the 'wordpress' plugin for TinyMCE. That will make it available to all. I'll make a patch later today.

#29 @adamsilverstein
11 years ago

great idea. let me know if you want me to hook that into the format list on the edit page, and what about post-standard instead of post-0 there?

#30 @rachelbaker
11 years ago

  • Cc rachelbaker removed

#31 in reply to: ↑ 28 @helen
11 years ago

  • Keywords easy-fix removed

Replying to azaozz:

The comment by @helen got me thinking: we may want to dynamically change/replace a TinyMCE body class for something else in the future. We can abstract this bit and add it to the 'wordpress' plugin for TinyMCE. That will make it available to all. I'll make a patch later today.

Anything on that?

#32 @wonderboymusic
11 years ago

OCD'd the whitespace for the new code

#33 @helen
11 years ago

  • Component changed from TinyMCE to Post Formats

@adamsilverstein
11 years ago

updated tinymce container class dynamically when changing post-format

#34 @adamsilverstein
11 years ago

23198.2.diff​ adds code to post-formats.js to swap out the post-format-(format) class added to the tinymce div; works with current tabs, easy to update when those go away. based on code suggestion from @azaozz

#35 follow-up: @azaozz
11 years ago

Some quick notes on 23198.2.diff​:

  • Still needs if ( typeof tinymce != 'undefined ) at the top.
  • Not sure it needs post-type- body class as well... Nothing to do with formats :)
  • Seems post-formats.js is loaded only on the Edit Post screen. Can be in post.js, no need to be separate file.

#36 in reply to: ↑ 35 @adamsilverstein
11 years ago

Replying to azaozz:

Some quick notes on 23198.2.diff​:

  • Still needs if ( typeof tinymce != 'undefined ) at the top.
  • Not sure it needs post-type- body class as well... Nothing to do with formats :)
  • Seems post-formats.js is loaded only on the Edit Post screen. Can be in post.js, no need to be separate file.

thanks for the review, will update.

#37 follow-up: @helen
11 years ago

Another question about the patch - why add the ID attribute for the slug? There's already a data-wp-format attribute.

#38 in reply to: ↑ 37 @adamsilverstein
11 years ago

Replying to helen:

Another question about the patch - why add the ID attribute for the slug? There's already a data-wp-format attribute.

will remove.

@adamsilverstein
11 years ago

cleanup as per suggestions

#39 @azaozz
11 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 23730:

Pass post format as a class to TinyMCE's body, props adamsilverstein, fixes #23198

Note: See TracTickets for help on using tickets.