WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#28458 closed defect (bug) (fixed)

wp.mce.view: simplify things a bit

Reported by: iseulde Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.9
Component: TinyMCE Keywords:
Focuses: javascript Cc:

Description

I'll use this ticket for a couple of things.
First thing: use the ready event for the playlist view instead of overwriting the render method.

Attachments (4)

28458.patch (1.8 KB) - added by iseulde 7 years ago.
28458.2.patch (2.9 KB) - added by iseulde 7 years ago.
28458.3.patch (17.2 KB) - added by iseulde 7 years ago.
28458.diff (18.0 KB) - added by wonderboymusic 7 years ago.

Download all attachments as: .zip

Change History (23)

@iseulde
7 years ago

#1 @iseulde
7 years ago

  • Keywords has-patch added

@iseulde
7 years ago

#2 @iseulde
7 years ago

(2) Removes className, because it's not used anywhere. Combines (1).

@iseulde
7 years ago

#3 @iseulde
7 years ago

(3) Added defaultConstructor with a toView method to parse shortcodes, since most views will be shortcodes. It can still be overwritten. Patch also has (1) and (2).

#4 @iseulde
7 years ago

Other ideas: might be good to detect whether there is an edit function and dynamically add a toolbar with an edit button, instead of adding a toolbar for every view in a template...

#5 @iseulde
7 years ago

Another idea: pass attributes, content and tag variables to getHtml for easy access. Right now you have to use this.shortcode.attrs.named.

#6 @iseulde
7 years ago

Editing a view could also be a lot easier if the API took care of $( node ).attr( 'data-wpview-text', window.encodeURIComponent( shortcode ) );...
I'd also be cool if the old shortcode is passed, so you don't have to window.decodeURIComponent( $( node ).attr('data-wpview-text') );.

#7 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 4.0

#8 follow-up: @wonderboymusic
7 years ago

@avryl: you read my mind on changing wp.mce.media to wp.mce.av

@wonderboymusic
7 years ago

#9 @wonderboymusic
7 years ago

28458.diff restores the shortcode property to the registered views for a/v/playlists. When ->edit() gets called, this is the base class, not the base.View class. I was getting JS errors when clicking to edit otherwise.

I will drop the rest in.

#10 @wonderboymusic
7 years ago

In 28680:

Simplify the creation of MCE views somewhat:

  • Rename the wp.mce.media mixin (which was named too ambiguously) to wp.mce.av.
  • Use the same technique for extending mixins for MCE base classes for views and their base.View property class
  • wp.mce.views.register() should have default constructor logic if one is not passed.

Props avryl.
See #28458.

#11 @gcorne
7 years ago

Everything seems to be working after [28680], but I have a couple thoughts on the code changes:

  1. I think that the type property should be reverted to shortcode. It can still default to being identical to the type as passed into register().
  1. I don't think that the creation of the wpview View constructor should be done in register(). Doing so takes some control away from the code registering the wpview.
  1. I wonder if we should rename the parameter constructor to wpview. The parameter isn't really a constructor; it's is a module expressed in the form of an object literal. The hard part is coming up with a name since there are two different types of views: 1) the wpview (aka content block?) and 2) the View that used to construct an individual instance of a wpview that is tied to a particular shortcode or other substring from the post content. Since the tinymce plugin is named wpview maybe that works?

So register() would end up looking something like:

	register: function( type, wpview ) {
		var defaults = {
			shortcode: type,
			View: {},
			toView: function( content ) {
				var match = wp.shortcode.next( this.shortcode, content );
				if ( ! match ) {
					return;
				}
				return {
					index: match.index,
					content: match.content,
					options: {
						shortcode: match.shortcode
					}
				};
			}
		};

		wpview = _.defaults( wpview, defaults );

		views[ type ] = wpview;
	},

And, the other bits would be adjusted accordingly.

#12 in reply to: ↑ 8 @iseulde
7 years ago

@avryl: you read my mind on changing wp.mce.media to wp.mce.av

:)

I don't think that the creation of the wpview View constructor should be done in register(). Doing so takes some control away from the code registering the wpview.

I think we should make it as easy as possible for plugins to register a view. Is there a case where you'd want to overwrite that? Just wondering. :)

Since the tinymce plugin is named wpview maybe that works?

Right now there are a bit too many terms for it... wpview, mceview, view, preview, content block... :) It's quite confusing.

#13 @wonderboymusic
7 years ago

In 28689:

As per @gcorne's suggestion, when calling wp.mce.views.register(), automatically set shortcode equal to the passed type in the set of default args instead of introducing a type property. It is still overrideable by the args that are passed.

See #28458.

#14 @wonderboymusic
7 years ago

I'll wait til a consensus on what should happen in register(), or til one of you makes a patch

#15 @wonderboymusic
7 years ago

anything else to do here?

#16 follow-up: @iseulde
7 years ago

Oh, yes. :)

#17 in reply to: ↑ 16 @ocean90
7 years ago

  • Keywords needs-patch added; has-patch removed

Replying to avryl:

Oh, yes. :)

Enough for 4.0, new ticket for 4.1?

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


7 years ago

#19 @DrewAPicture
7 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

Let's move to new tickets for new issues and call this fixed.

Note: See TracTickets for help on using tickets.