WordPress.org

Make WordPress Core

#23198 closed enhancement (fixed)

Pass post format as a class to TinyMCE init

Reported by: helen Owned by: 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 15 months ago.
23198-2.patch (781 bytes) - added by azaozz 15 months ago.
23198-3.diff (1.7 KB) - added by adamsilverstein 15 months ago.
add-WP_Error.diff (440 bytes) - added by adamsilverstein 15 months ago.
adds WP_Error return to get_post_format
23198-4.diff (2.4 KB) - added by adamsilverstein 15 months ago.
cleared up phpdoc
23198.diff (2.2 KB) - added by wonderboymusic 15 months ago.
23198.2.diff (2.6 KB) - added by adamsilverstein 14 months ago.
updated tinymce container class dynamically when changing post-format
23198.3.diff (760 bytes) - added by adamsilverstein 13 months ago.
cleanup as per suggestions

Download all attachments as: .zip

Change History (47)

comment:1 sabreuse15 months ago

  • Cc sabreuse added

comment:2 adamsilverstein15 months ago

  • Cc ADAMSILVERSTEIN@… added

comment:3 WraithKenny15 months ago

  • Cc Ken@… added

comment:4 alex-ye15 months ago

  • Cc nashwan.doaqan@… added

azaozz15 months ago

comment:5 azaozz15 months ago

Does this need anything more than 23198.patch?

comment:6 helen15 months ago

Well, needs to be format, not status :)

comment:7 azaozz15 months 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 :)

azaozz15 months ago

adamsilverstein15 months ago

comment:8 follow-up: adamsilverstein15 months 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.

comment:9 SergeyBiryukov15 months ago

  • Keywords has-patch added; needs-patch removed

comment:10 in reply to: ↑ 8 azaozz15 months 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.

comment:11 adamsilverstein15 months 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'));


adamsilverstein15 months ago

adds WP_Error return to get_post_format

comment:12 follow-up: adamsilverstein15 months 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.

comment:13 in reply to: ↑ 12 azaozz15 months 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 15 months ago by azaozz (previous) (diff)

adamsilverstein15 months ago

cleared up phpdoc

comment:14 adamsilverstein15 months 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)

comment:15 adamsilverstein15 months 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.


comment:16 rachelbaker15 months ago

  • Cc rachelbaker added

comment:17 azaozz15 months ago

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

comment:18 follow-up: adamsilverstein15 months ago

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

comment:19 follow-up: WraithKenny15 months 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()?

comment:20 adamsilverstein15 months 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?

comment:21 in reply to: ↑ 18 ; follow-up: azaozz15 months ago

Replying to adamsilverstein:

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

comment:22 in reply to: ↑ 19 azaozz15 months 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.

comment:23 follow-up: helen15 months 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 :)

comment:24 in reply to: ↑ 21 adamsilverstein15 months ago

Replying to azaozz:

Replying to adamsilverstein:

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

ok, i can do that.

comment:25 in reply to: ↑ 23 ; follow-up: adamsilverstein15 months 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!

comment:26 in reply to: ↑ 25 azaozz15 months 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 15 months ago by azaozz (previous) (diff)

comment:27 adamsilverstein15 months 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">

comment:28 follow-up: azaozz15 months 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.

comment:29 adamsilverstein15 months 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?

comment:30 rachelbaker15 months ago

  • Cc rachelbaker removed

comment:31 in reply to: ↑ 28 helen15 months 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?

wonderboymusic15 months ago

comment:32 wonderboymusic15 months ago

OCD'd the whitespace for the new code

comment:33 helen14 months ago

  • Component changed from TinyMCE to Post Formats

adamsilverstein14 months ago

updated tinymce container class dynamically when changing post-format

comment:34 adamsilverstein14 months 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

comment:35 follow-up: azaozz14 months 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.

comment:36 in reply to: ↑ 35 adamsilverstein14 months 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.

comment:37 follow-up: helen13 months ago

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

comment:38 in reply to: ↑ 37 adamsilverstein13 months 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.

adamsilverstein13 months ago

cleanup as per suggestions

comment:39 azaozz13 months 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.