Add/805-playlist-refactor-2 by antpb · Pull Request #9169 · WordPress/gutenberg

antpb · 2018-08-20T15:11:04Z

This is a clean PR to add the playlist block to Gutenberg. There were merge conflicts that resulted in me needing to create a new branch based on the newest version of Gutenberg. Previous conversation can be found here: #7126

There is one bug currently with notices where the mismatched type error shows 4 of the same notice. Working through that and we should be good for another review.

CC: @karmatosed @noisysocks

initial commit

starting playlist over

….may need core mod >_< storing ids in comment as ids attribute. moving to render callback....may need core mod >_<

…ttributes error

…mmits from airport terminals ✈

This simply moves the playlist block in the list of blocks imported and registered in core-blocks/index.js.

This commit generally accomplishes a few thing: - Removes any unused variables and functions - Ensuring all used functions and variables are declared - Simplifies some variable declarations - Fixing spacing to conform to WP style

This fixes up code style issues, adds correct docblocks, and removes unneccessary script rendering which isn't necessary during server rendering.

This wraps the shortcode output in a `figure` element with a class of `wp-block-playlist` applied so we can style the block accordingly.

This adds an editor.scss file to the playlist block that removes margins from `.wp-playlist-block` elements, to avoid margins being added as a default style by browsers.

This adds an align attribute to the playlist block and adds the proper alignment class to the markup on the front end to support alignments. This also deletes the style.scss file, which now seems unnecessary.

@joemcgill

Playlist block updates @joemcgill fixed a ton of formatting and bugs. :D

aduth · 2018-08-28T19:20:04Z

packages/block-library/src/playlist/edit.js

+	}
+
+	/**
+	* The function is used after the editor has enqueued playlist dependencies.


Nitpicks:

Proper JSDoc alignment should have space before each * of the body, after the tab

See: http://usejsdoc.org/about-getting-started.html

The newline between the JSDoc block and the function declaration may prevent it from being parsed correctly as associated. I suggest removing it.

aduth · 2018-08-28T19:22:40Z

packages/block-library/src/playlist/index.js

+
+	category: 'common',
+
+	attributes: {


It is a duplication, yes. You shouldn't need to define attributes here. I'll need to take a closer look to understand where things aren't working correctly.

aduth · 2018-08-28T19:31:45Z

packages/block-library/src/playlist/edit.js

+	onSelectMedia( media ) {
+		const { setAttributes } = this.props;
+		//check if there are returned media items and set attributes when there are
+		if ( media && media[ 0 ].url ) {


When will media be falsey?

I don't believe it will ever be unless there is an error maybe. We also have a similar check in the Audio block. I was matching that for consistency on the chance there was an instance I was unaware of: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/audio/edit.js#L88

aduth · 2018-08-28T19:32:11Z

packages/block-library/src/playlist/edit.js

+	onSelectMedia( media ) {
+		const { setAttributes } = this.props;
+		//check if there are returned media items and set attributes when there are
+		if ( media && media[ 0 ].url ) {


Why do we test for a url property which we have no intention of using? Do we care about the property, or just that media.length > 0 ?

we shouldnt. I've removed the condition

aduth · 2018-08-28T19:44:57Z

packages/block-library/src/playlist/edit.js

+	getMimeBaseType( type ) {
+		const { noticeOperations } = this.props;
+		if ( ! type ) {
+			noticeOperations.createErrorNotice( 'MIME Type could not be determined' );


Nitpick: Why is "Type" capitalized?

Noting that this message will be shown multiple times if multiple files cannot have their mime types determined.

I've fixed the capitalization. Will think through the multiple messages bit. Can create an issue for it also after merge if we want to save that for future work.

aduth · 2018-08-28T19:45:25Z

packages/block-library/src/playlist/edit.js

+	uploadFromFiles( event ) {
+		const { noticeOperations } = this.props;
+		if ( ! event.target.files ) {
+			noticeOperations.createErrorNotice( 'Files list provided is empty' );


I sense that these should be localized?

aduth · 2018-08-28T19:46:39Z

packages/block-library/src/playlist/edit.js

+		this.onUploadFiles( event.target.files );
+	}
+
+	getMimeBaseType( type ) {


Minor: I don't expect a function prefixed with "get", particularly one like this which could theoretically be made generic (mime type has meaning outside the playlist block) to have a side-effect such as causing a notice to be created.

aduth · 2018-08-28T20:00:20Z

packages/block-library/src/playlist/index.js

+
+	category: 'common',
+
+	attributes: {


I tested locally and I see no obvious issues when removing attributes from this JavaScript file. Can you elaborate on what you're seeing?

aduth · 2018-08-28T20:04:31Z

packages/editor/src/utils/media-upload/media-upload.js


 	// Allowed type specified by consumer
 	const isAllowedType = ( fileType ) => {
-		return ( allowedType === '*' ) || startsWith( fileType, `${ allowedType }/` );


I'm not really sure I understand.

What failures were you seeing without these changes? You're not specifying * so I don't see why it would have any impact to leave it.

Regardless of whether a * is sensible, it exists and is already in use by the Files block. If we want to remove it, we need to remove any references to it. Speaking personally, I don't really like * as the wildcard. It seems like if someone wants any file to be uploaded, they should just omit the allowedType property altogether. But then again, this change has nothing to do with a Playlist block implementation and seems like it should be a separate effort.

aduth · 2018-08-28T20:07:01Z

packages/components/src/server-side-render/index.js

 		if ( ! isEqual( prevProps, this.props ) ) {
 			this.fetch( this.props );
 		}
+		if ( this.state.response !== prevState.response ) {


It's a bit awkward to call a prop in response to a state change in componentDidUpdate, since a component manages its own state and begs the question why isn't the prop called at the time we're calling setState ? Our current implementation of ServerSideRender#fetch doesn't make this very easy to refactor, though...

Is this a merge blocker? If not can we bring it to an issue after? I think there would have to be an alteration of ServerSideRender to accomplish this. I fear it may introduce more bugs

…es for '*'

gziolo · 2018-08-30T05:40:13Z

packages/block-library/src/playlist/edit.js

+	*/
+
+	initializePlaylist() {
+		window.wp.playlist.initialize();


@youknowriad, this block depends on both WordPress global wp.playlist and ServerSideRender component which is very backend tied. We need to do something about it. There should be reusable blocks which work everywhere and blocks which are reusable but WordPress specific. If we would draw this distinction we wouldn't have to worry about Code and Freeform anymore.

yes, a first step would be to move it ouside of the packages like html or freeform. Later on, we'd need to rewrite wp.playlist as a Gutenberg package and then move the block to the packages as well.

gziolo · 2018-08-30T05:45:25Z

lib/client-assets.php

 		'post' => $post->ID,
 	) );
 	wp_enqueue_editor();
+	wp_playlist_scripts( 'audio' );


Shouldn't wp.playlist to be listed as a dependency of wp-block-library? It's referenced it in the code so I would assume that it needs to be.

gziolo · 2018-08-30T06:44:02Z

Did you check how much more data users will have to load in their browser to be able to use this block? In particular, how much weigh all the files enqueued with those two lines:

wp_playlist_scripts( 'audio' );
wp_playlist_scripts( 'video' );

antpb · 2018-08-30T08:53:53Z

@gziolo there was some previous discussion about this in an earlier review. It adds about 110-120kb : #7126 (comment)

aduth · 2018-08-30T12:55:38Z

The additional weight is significant, particularly for a block which won't always be commonly used by a large subset of users.

I wonder if it's something where we should employ the technique as we did for the HTML block and CodeMirror:

gutenberg/components/code-editor/index.js

Lines 28 to 58 in eac95b5

    
           function loadScript() { 
        
           	return new Promise( ( resolve, reject ) => { 
        
           		const handles = [ 'wp-codemirror', 'code-editor', 'htmlhint', 'csslint', 'jshint' ]; 
        
           		// Don't load htmlhint-kses unless we need it 
        
           		if ( window._wpGutenbergCodeEditorSettings.htmlhint.kses ) { 
        
           			handles.push( 'htmlhint-kses' ); 
        
           		} 
        
           		const script = document.createElement( 'script' ); 
        
           		script.src = `${ siteURL }/wp-admin/load-scripts.php?load=${ handles.join( ',' ) }`; 
        
           		script.onload = resolve; 
        
           		script.onerror = reject; 
        
           		document.head.appendChild( script ); 
        
           	} ); 
        
           } 
        
           function loadStyle() { 
        
           	return new Promise( ( resolve, reject ) => { 
        
           		const handles = [ 'wp-codemirror', 'code-editor' ]; 
        
           		const style = document.createElement( 'link' ); 
        
           		style.rel = 'stylesheet'; 
        
           		style.href = `${ siteURL }/wp-admin/load-styles.php?load=${ handles.join( ',' ) }`; 
        
           		style.onload = resolve; 
        
           		style.onerror = reject; 
        
           		document.head.appendChild( style ); 
        
           	} ); 
        
           }

Also related: #8017

aduth · 2018-08-30T18:50:52Z

Raising other options to explore / consider:

Simplify what we're displaying as a preview in the editor. Avoid pulling in any dependencies and just render a simplified list of file names (no in-browser preview in editor).
Make it the responsibility of ServerSideRender to bundle the relevant scripts, whether that's inline or as part of an embedded iframe.

antpb · 2018-08-30T18:58:46Z

@aduth After digging into this a bit today, I remember the thought process in adding the playlist enqueues to client-assets. If you remove the script calls from that file and create a playlist block, the block responds that it has encountered an error and cannot load. Saving/publishing the post and refreshing the editor magically allows it to render with it's dependencies.

Something is not triggering in ServerSideRender at the right time to enqueue the dependencies. Still digging through this, but I think the best path forward is the lazy load solution proposed earlier with a state that says that it has dependencies before render.

joemcgill · 2018-09-13T20:06:42Z

Related to #805

…nt-assets.php

gziolo · 2019-04-24T12:49:13Z

@melchoyce @mapk @jasmussen @karmatosed @youknowriad - what's the plan regarding this block? It isn't listed as the block planned for phase 2.

mapk · 2019-05-08T17:34:29Z

what's the plan regarding this block? It isn't listed as the block planned for phase 2.

Definitely not on our list of priorities right now.

I'm going to close this because it hasn't been touched since September 2018. But I don't want to discourage contributors from working on something they're passionate about. If anyone feels like refactoring this and opening it back up, go ahead. 😄

antpb and others added 30 commits May 30, 2018 20:42

initial commit

ef14b27

initial commit

playlist init

963e1c4

starting playlist over

storing ids in comment as ids attribute. moving to render callback...…

56d89ef

….may need core mod >_< storing ids in comment as ids attribute. moving to render callback....may need core mod >_<

storing ids in comment as ids attribute. moving to render callback...…

8024ef6

….may need core mod >_< storing ids in comment as ids attribute. moving to render callback....may need core mod >_<

playlist_block_callback_dependencies function. adds index.php

2b5a083

cleaning up attributes and logging code. removes mediaelement component

f35d9ef

serversiderender component being used now in edit...workign through a…

387a964

…ttributes error

removes client-asset changes meant for mediaelement component

b3f4f67

adds media-upload changes to support playlist modal

67e55d3

adds type attribute for video/audio conditional for the render callback

a41060d

fixed edit state and now when you edit post, it acts as expected - co…

bdb9040

…mmits from airport terminals ✈

Adjusted some of the wording to include video language...not just audio

204421f

Group coding by antpb - noisysocks - soean

ca9776f

Fixing noisysocks suggested changes

9289d24

Load playlist block alphabetically

56b7d46

This simply moves the playlist block in the list of blocks imported and registered in core-blocks/index.js.

Clean up lint errors

b8d8e1a

This commit generally accomplishes a few thing: - Removes any unused variables and functions - Ensuring all used functions and variables are declared - Simplifies some variable declarations - Fixing spacing to conform to WP style

Clean up dynamic rendering PHP code

bc65ce2

This fixes up code style issues, adds correct docblocks, and removes unneccessary script rendering which isn't necessary during server rendering.

Wrap server rendered playlists in figure element

52437ff

This wraps the shortcode output in a `figure` element with a class of `wp-block-playlist` applied so we can style the block accordingly.

Remove non-playlist styles from the playlist stylesheet.

dca6449

Fix playlist margins in the editor

4561b5c

This adds an editor.scss file to the playlist block that removes margins from `.wp-playlist-block` elements, to avoid margins being added as a default style by browsers.

Fix alignment support for the playlist block

f7d7cfe

This adds an align attribute to the playlist block and adds the proper alignment class to the markup on the front end to support alignments. This also deletes the style.scss file, which now seems unnecessary.

Merge pull request #1 from joemcgill/add/playlist-block

e74ab60

Playlist block updates @joemcgill fixed a ton of formatting and bugs. :D

Fixed edit button to auto populate media modal

4c8efb9

cleaned up spacing issues

7dbe010

fixes Soean's suggestions and adding attributes to fix settings issue

ed19672

removes some silly test code

9f03842

Add controls and fix lint errors

936732b

Merge branch 'master' into add/playlist-block-805

92c2306

removes SettingsView from Playlist Media view

87d9fee

fixes gallery mutiple bug - Nice find @noisysocks !

3d5cabb

antpb added 6 commits August 24, 2018 12:18

removes isEditing state completely

094b917

adds initializePlaylist documentation

d4ab87a

pulls getMimeBaseType out of the component

b5743f3

adds condition to verify type is passed from the files uploaded

06d0e90

adds check to if files list exists

f3b25c8

fixes JS tests

32fb80d

aduth reviewed Aug 28, 2018

View reviewed changes

antpb added 5 commits August 29, 2018 10:07

removes attributes from index.js to make things tidy

7ebde3c

fixes comment spacing for JS standards

3c60c35

removes condition

612ddce

fixes error message capitalization

c2e9ec5

adds localization, fixes gallery view defaults, and fixes upload issu…

bb9ae00

…es for '*'

gziolo reviewed Aug 30, 2018

View reviewed changes

aduth mentioned this pull request Aug 30, 2018

Babel Plugin Async Load: Lazy load react components when necessary #8017

Closed

aduth mentioned this pull request Sep 14, 2018

Media: Refactor MediaUpload, MediaPlaceholder and mediaUpload to support arrays with multiple supported types. #9707

Merged

antpb mentioned this pull request Oct 3, 2018

Gallery: figure out way to fallback to shortcode #1451

Closed

conditionally loads playlist dependencies instead of doing it in clie…

4119c5a

…nt-assets.php

Soean mentioned this pull request Jan 14, 2019

Add Block: Playlist #805

Closed

gziolo added Needs Decision Needs a decision to be actionable or relevant and removed [Status] In Progress Tracking issues with work in progress labels Apr 24, 2019

mapk closed this May 8, 2019

Conversation

antpb commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Aug 30, 2018

Uh oh!

antpb commented Aug 30, 2018

Uh oh!

aduth commented Aug 30, 2018

Uh oh!

aduth commented Aug 30, 2018

Uh oh!

antpb commented Aug 30, 2018

Uh oh!

joemcgill commented Sep 13, 2018

Uh oh!

gziolo commented Apr 24, 2019

Uh oh!

mapk commented May 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants