Video: Redux state changes for custom video poster #11633

donnapep · 2017-02-28T13:35:01Z

This PR separates out the Redux state changes from PR #11389, which adds support for selecting or uploading a custom image to serve as the poster for a video.

Post discussing Calypso-based VideoPress poster chooser can be found at pxWta-JO-p2 (internal ref).

Upload poster endpoint

matticbot · 2017-02-28T13:35:03Z

Test live: https://calypso.live/?branch=add/video-editor-redux-state

donnapep · 2017-03-06T16:47:56Z

@marcinbot @lamosty @gwwar Tagging you guys to review this one as it looks like you worked on the image editor. This is the video equivalent of that. Thx!

gwwar · 2017-03-08T19:49:24Z

client/lib/wpcom-undocumented/lib/undocumented.js

+ * @param {Function} fn Function to invoke when request is complete
+ * @returns {Promise} A promise that resolves when the request is complete
+ */
+Undocumented.prototype.updateVideoPoster = function( videoId, data, fn ) {


Is this a new wpcom only endpoint? Did we have any plans for Jetpack support?

I'll very much encourage using the new data layer for any and all new wpcom endpoints and would be happy to walk you through it. In the meantime, check out the API layer and HTTP layer readmes.

@gwwar This is a WPCOM only endpoint, as VideoPress is a WPCOM feature. Jetpack uploads its videos to WPCOM for all VideoPress requests, not the other way around.

gwwar · 2017-03-08T19:51:15Z

client/state/ui/editor/video-editor/actions.js

+ * @param  {Object}   params  Poster data
+ * @return {Function}         Action thunk
+ */
+export const updateVideoEditorPoster = ( guid, params ) => {


I think it'd be useful to describe the params shape in the jsdoc

Also, I think returning a thunk here is fine, though if you're up for it, there's a new data-layer pattern that we can use.

The upside is that testing becomes a lot easier, since we can usually dispatch a simple action on the dev console to trigger the entire the request. Maybe something like

dispatch( { type: VIDEO_EDITOR_POSTER_UPDATE, videoId, params } );

Ping me or @dmsnell for help on that

gwwar · 2017-03-08T19:56:44Z

client/state/ui/editor/video-editor/reducer.js

+	hasScriptLoadError,
+	poster,
+	posterIsUpdating,
+	posterIsUpdated,


What's the difference here between posterIsUpdating and posterIsUpdated?

@gwwar When the poster is being updated on the server, posterIsUpdating gets set and the UI will show a spinner.

I had to introduce a separate posterIsUpdated because I needed to know if the poster had been successfully updated, and if so, then close out the video editor and return to the previous screen - https://github.com/Automattic/wp-calypso/pull/11648/files#diff-35b626cb9b7af72412a7ab302d8fc3f6R67

dbtlr

Not sure if the commas matter as much as I made them out to, but I would definitely make sure you fix how the form is posting parameters.

dbtlr · 2017-03-09T13:49:34Z

client/lib/wpcom-undocumented/lib/undocumented.js

+		];
+	}
+
+	if ( 'at_time' in data ) {


I would make this with an elseif, since you can't send both (the logic will always take the at_time first on the server side). Also, you should have an else at the end, that if neither is present, it will cause this to not post to the API.

@dbtlr the way we would normally prefer to handle this isn't with a dangling else at the end but rather with an early abort from the function call. you will find that we have very few else structures in Calypso.

also, in general, I would strongly encourage working together on this to use the new data layer instead of creating a new wpcom.undocumented function

@dmsnell Re: using the data layer. It's currently in progress. 😉

@donnapep please don't hesitate to discuss the work! the data layer is still very new and we're still iterating on the developer experience, so I would value your input

dbtlr · 2017-03-09T13:53:45Z

client/state/ui/editor/test/reducer.js

@@ -16,7 +16,8 @@ describe( 'reducer', () => {
 			'showDrafts',
 			'lastDraft',
 			'contactForm',
-			'imageEditor'
+			'imageEditor',
+			'videoEditor',


Is it proper to have a final comma in a JavaScript array?

in Calypso it is yeah. we encourage it for the effects on diffs

dbtlr · 2017-03-09T13:54:03Z

client/state/ui/editor/actions.js

@@ -13,7 +13,8 @@ export const MODAL_VIEW_STATS = {
 	[ ModalViews.LIST ]: 'view_list',
 	[ ModalViews.DETAIL ]: 'view_detail',
 	[ ModalViews.GALLERY ]: 'view_gallery',
-	[ ModalViews.IMAGE_EDITOR ]: 'view_edit'
+	[ ModalViews.IMAGE_EDITOR ]: 'view_edit',
+	[ ModalViews.VIDEO_EDITOR ]: 'view_edit',


dbtlr · 2017-03-09T13:54:25Z

client/state/ui/editor/video-editor/actions.js

+	VIDEO_EDITOR_SCRIPT_LOAD_ERROR,
+	VIDEO_EDITOR_STATE_RESET,
+	VIDEO_EDITOR_STATE_RESET_POSTER,
+	VIDEO_EDITOR_VIDEO_HAS_LOADED,


@dbtlr I like to do this for objects so that if you need to add another property later, you don't get the extra diff on the preceding line. I see this being done occasionally in Calypso, but it's not part of the official style guide.

donnapep · 2017-03-10T12:41:17Z

client/state/data-layer/wpcom/videos/poster/index.js

+		method: 'POST',
+		path: `/videos/${ action.videoId }/poster`,
+		body: 'at_time' in action.params ? action.params : undefined,
+		formData: 'file' in action.params ? [ [ 'poster', action.params.file ] ] : undefined,


@dmsnell formData does not appear to be sent with the POST request. Before I get into some deeper debugging, does what I have here so far look OK?

possibly a bug? I will look into this

A fix for this can be found in PR #12135, although it still needs more testing to ensure nothing breaks. Thx!

donnapep · 2017-03-10T12:43:57Z

client/state/data-layer/wpcom/videos/poster/index.js

+		onFailure: action,
+	} ) );
+
+	return next( action );


@dmsnell I don't quite get when I should use next vs. dispatch. And in what sort of circumstances would I want to do neither of those things?

next() is going to be used when we're passing the given action along the middleware chain. maybe we intercept post editor updates and want to make sure that it's capital_P_dangit. we could intercept that action, filter post_content, and then we would want to pass it along to the next link in the chain with next() - this is like add_filter() in WP.

on the other hand, we might prefer to trigger some side-effect or corresponding action. maybe every time we see Wordpress we want to bump some state. in this case we would pass along the original untouched action to next() but since we're firing off a new action altogether we want it to start at the front of the middleware stack so we'll fire off dispatch( bumpStat( … ) ) - akin to add_action in WP.

@coderkevin just since we recently discussed actions and filters I thought I'd poke you on this ^^^

@dmsnell is there ever a case where we wouldn't want to pass the action along the middleware chain? As is we'll get weird oddities like the dispatch action not being recorded by redux dev tools if we omit return next( action ) in addition to missing any other side effect middleware that was lower on the chain.

For example in console if you dispatch:

dispatch( { type: 'ACCOUNT_RECOVERY_RESET_REQUEST' } );

We'll see the other actions recorded but not the initial action request :

https://github.com/Automattic/wp-calypso/pull/11699/files#diff-2c98cce53eb8b5257f1bbca88632820cR17
cc @southp

@gwwar the actions should be recorded on their first dispatch() so they should appear even if we don't pass them along (would be a bug if this isn't the case and we would probably just need to rearrange the middleware)

times we wouldn't want to pass along the action? maybe so. we might want to cull actions from hitting the reducers or system for some reason. not sure off-hand what these might be but it seams very plausible to me.

I think this is all well and good, but it's a pretty privileged operation here. I feel more thought should go into what should be allowed for which Calypso extensions when we start accepting more outside contributions. (Note: this is merely an observation and no action is required for this on this current PR.)

donnapep · 2017-03-10T12:45:22Z

client/state/data-layer/wpcom/videos/poster/index.js

+};
+
+export default {
+	[ VIDEO_EDITOR_POSTER_UPDATE ]: [ dispatchRequest( updateVideoEditorPoster, updateVideoPosterSuccess, updateVideoPosterError ) ],


@dmsnell It seems that I needed to add a new action to be used explicitly by the data layer. Is this true in general?

@donnapep I'm not sure what your question is. the data layer latches into existing actions which feed through the system. it handle side-effects and data fetching.

the idea is that Calypso can function as best as possible even if no data gets synchronized. so maybe we fire an event VIDEO_POSTER_UPDATE which specifies a video and a frame for the post (is that what this is about?) then Calypso can hold onto that state the same way it would if it had just loaded that data from the API.

in this case the data layer sees that action fly by and it says to itself, "self, I know that when someone changes the video poster we also need to let WordPress.com know so that it persists and propagates, I will do just that"

now, once we have fully uploaded the data and get some response we can make a judgement. if it's as simple as saying, "use frame 1337 for the poster" then if we get a positive response back from the server showing that it stored 1337 and our state already shows 1337 (because Redux state operates independently of the middleware) then we don't actually need to do a thing. no changes are required. on the other hand, if it comes back with the wrong frame number we may need to respond in one of many possible ways: retry the operation (Calypso meanwhile is unaware of the failure), report the failure to the user, or just force Calypso to update (overwriting the customer's choice).

please let me know if this is addressing your question!

dmsnell · 2017-03-10T21:25:23Z

thanks for your rework on this @donnapep!

instead of diving into the details at this moment I'd like to raise some higher-level questions:

what is the purpose of storing data points like isLoading and isUpdating? what visual needs do we have for their representation? is it possible that we can already represent those values without storing additional state?
what is the purpose of having _SUCCESS and _FAILURE actions? I've been pushing hard to keep those out of the data layer and instead write code that focusses on the actual actions being done in the system: ADD_POST instead of POST_RECEIVE_SUCCESS for example. Although I think it's harder at the time we design the code to think about doing without these _SUCCESS/FAILURE actions I also believe that it leads to stronger code (because we decouple the state tree from the API) and smaller sprawl (because we reuse existing code).
there's an additional function we can add to dispatchRequest() which will be called on progress events. if we need to represent something like percentage of completed upload we can do that specifically with the data layer in an elegant way
we may have found a bug if formData isn't coming through. I'll try to look into this

doing well - I'm excited to have what I think is our first data uploader in the data layer!

donnapep · 2017-03-13T10:29:11Z

@dmsnell

what is the purpose of storing data points like isLoading and isUpdating? what visual needs do we have for their representation?

isLoading is used to check whether or not the video has fully loaded. If it hasn't, the Select Frame button will be disabled since it doesn't make sense to select a frame of a video that hasn't loaded yet. isUpdating shows a spinner (or perhaps this will now change to a progress bar) while the poster is being updated.

what is the purpose of having _SUCCESS and _FAILURE actions?

We'd like to surface an error message if the poster failed to update. But if the update succeeds, then the poster URL is returned by the endpoint and the thumbnail for the video is updated.

there's an additional function we can add to dispatchRequest() which will be called on progress events. if we need to represent something like percentage of completed upload we can do that specifically with the data layer in an elegant way

I saw that. This may work when uploading a custom image, although not when selecting a particular frame. I'll play around with it some.

we may have found a bug if formData isn't coming through. I'll try to look into this

Sounds good. Cheers!

dmsnell · 2017-03-13T17:51:45Z

isLoading is used to check whether or not the video has fully loaded

don't we already get an event dispatched at the point the video has fully loaded? are we showing the customer some indicator that it's loading? this is an upload, right? is there a reason to prefer a binary isLoading/isNotLoading over a progress indicator with percentage of upload?

isUpdating shows a spinner

is this consistent with other Calypso idioms? I thought we normally assume that an operation works and then only undo if it fails. is there something fundamentally different about the poster calling for the behavior? is setting the poster frame a long operation that warrants showing some process state?

We'd like to surface an error message if the poster failed to update.

Here is where the data layer is designed to support Calypso design. If our goal is to surface an error message, then why not simply surface an error message? We have an action already for that - createNotice()…

if the update succeeds, then the poster URL is returned by the endpoint and the thumbnail for the video is updated.

…continuing on that thought, if our intention is to store the URL for a poster frame, why not use an action that mirrors that intention - MEDIA_ASSIGN_POSTER_FRAME or something like that. If we design our actions to be independent of the specifics of the API then we can communicate much more clearly in the code. It will also make it much easier to test with from within the console.

donnapep · 2017-03-14T15:39:54Z

don't we already get an event dispatched at the point the video has fully loaded?

Nope, not for VideoPress videos. This will be added by a separate PR in this commit 68a9d5f

are we showing the customer some indicator that it's loading?

Yes, but not as part of Calypso. VideoPress controls the UI for this.

this is an upload, right? is there a reason to prefer a binary isLoading/isNotLoading over a progress indicator with percentage of upload?

Nope, not an upload. This is just to initially display the video in the video editor. Once the video is loaded, then we can enable the button that allows the user to select a specific frame to use as the poster (or they can also choose to upload their own image instead of choosing a frame, this is where upload comes into play).

I think I've done a poor job of communicating the context for this PR. To get a better idea of the background, the master PR #11389 might explain it better.

is this consistent with other Calypso idioms? I thought we normally assume that an operation works and then only undo if it fails.

Honestly, I'm not sure. That's why I rely on reviewers like yourself to school me. 😉 I will say that video editor is largely modelled after image editor. With image editor, if the image can't be updated then an error message is shown onscreen. For consistency, video editor does a similar thing if the poster couldn't be updated. There's nothing to really "undo" though when updating the poster.

is there something fundamentally different about the poster calling for the behavior? is setting the > poster frame a long operation that warrants showing some process state?

Uploading the image can take awhile, especially for larger images. Selecting a particular frame is generally faster. I do think the upload warrants an indicator (thinking to potentially switch to a progress bar instead of a spinner here), but perhaps nothing is needed for selecting a frame.

Here is where the data layer is designed to support Calypso design. If our goal is to surface an error message, then why not simply surface an error message? We have an action already for that - createNotice()…

Thanks! This is worth investigating.

…continuing on that thought, if our intention is to store the URL for a poster frame, why not use an action that mirrors that intention - MEDIA_ASSIGN_POSTER_FRAME or something like that. If we design our actions to be independent of the specifics of the API then we can communicate much more clearly in the code. It will also make it much easier to test with from within the console.

I dunno. Maybe. But what if I wanted to do something else after the poster is updated in addition to assigning the poster frame URL? Now my action no longer tells the whole story and is a bit of a misnomer. I find this naming convention a bit less future-proof.

@dmsnell

dmsnell · 2017-03-14T19:18:15Z

But what if I wanted to do something else after the poster is updated in addition to assigning the poster frame URL? Now my action no longer tells the whole story and is a bit of a misnomer. I find this naming convention a bit less future-proof.

If we need to do more we can dispatch more actions. The point is to keep our actions specific to the intentions they convey. Ideally, if we have multiples visual changes that need to happen in response to the event then we will find some aptly-named action (such as SET_POSTER_FRAME) being listened for in multiple reducers. The predominant model I find is is one big reducer pasting in some GET_THING_SUCCESS action and it changes multiple independent state values.

dmsnell · 2017-03-17T18:52:33Z

client/state/ui/editor/video-editor/reducer.js

+ *
+ * @param  {Object} state  Current state
+ * @param  {Object} action Action object
+ * @return {Object}        Updated state


dmsnell · 2017-03-17T18:54:30Z

client/state/ui/editor/video-editor/reducer.js

+	}
+
+	return state;
+};


in this reducer, it looks like we only have a single true value - when the action type is VIDEO_EDITOR_SHOW_ERROR which could be much more succinctly captured in the following…

export const hasPosterUpdateError = ( state = false, { type } ) => type === VIDEO_EDITOR_SHOW_ERROR;

further, what's the point of storing a boolean status value for hasError? do we not store what kind of error it is? what value is there in this on/off state?

In this case, I don't think we necessarily care what the error is because it's not being used. We only care that the poster update wasn't successful, so that we can in turn communicate that to the user. I suppose we could log the specific error, but other than that it wouldn't be used.

dmsnell · 2017-03-17T18:55:51Z

client/state/ui/editor/reducer.js

@@ -33,6 +34,7 @@ export function postId( state = null, action ) {
 export default combineReducers( {
 	postId,
 	imageEditor,
+	videoEditor,


it appears like the video editor state has no reference to any particular media id or site or anything. it's "whatever is out front in the editor" but what happens if we change sites or change videos which we are working on? won't we have race-conditions with network requests on the wire?

The video editor is a modal that is opened from the media library modal. Media library modal handles passing the particular video and everything else the video editor needs to know. And because it's a modal, site switching should be a non-issue. Hope that helps clarify and makes sense.

dmsnell · 2017-03-17T19:00:25Z

client/state/ui/editor/video-editor/selectors.js

+ */
+export function hasPosterUpdateError( state ) {
+	return state.ui.editor.videoEditor.hasPosterUpdateError;
+}


please note that we are putting all new selectors into the flat state/selectors directory as part of a new search-based selector system (check the devdocs). they belong one for each file where the function name matches the file name except for casing

// has-poster-update-error.js export default function hasPosterUpdateError( state ) { … }

dmsnell

among other things - the selectors definitely need to move into the new flat selectors directory

donnapep · 2017-03-22T15:23:49Z

@dmsnell The ball's back in your court!

dmsnell · 2017-03-23T04:04:14Z

client/state/ui/editor/video-editor/reducer.js

+	switch ( action.type ) {
+		case VIDEO_EDITOR_SET_POSTER_URL:
+		case VIDEO_EDITOR_SHOW_ERROR:
+			return true;


is the poster really updated if we got an error? seems like a semantic mismatch between the data point and its use

dmsnell · 2017-03-23T04:07:18Z

client/state/data-layer/wpcom/videos/poster/index.js

+ * Updates the poster for a video.
+ *
+ * @param  {Object} store  Redux store
+ * @param  {Object} action  Action object


nitpick: we have extra spaces here

dmsnell · 2017-03-23T04:08:09Z

client/state/data-layer/wpcom/videos/poster/index.js

+		return next( action );
+	}
+
+	const { atTime, file } = pick( action.params, [ 'atTime', 'file' ] );


I don't think pick is necessary here as the destructuring should already do exactly what we want…

const { atTime, file } = action.params;

dmsnell · 2017-03-23T04:12:47Z

client/state/data-layer/wpcom/videos/poster/index.js

+};
+
+export const updatePosterError = ( { dispatch } ) => {
+	dispatch( showError() );


this error concept is still bugging me. we don't actually show an error do we? it seems like all we really do is indicate that the poster is no longer updating. is the error coming later?

is there another way to represent the poster URL which would indicate if it's reset or set? on error, instead of sending an error action, would it make sense to revert the URL to its previous state, or an empty string? if so, we could instead dispatch something like setPosterUrl( previousURL )

We do actually surface an error message to the user in the case where the poster could not be updated. To be clear, the new poster URL gets generated server-side and is sent back to the client on a successful update. If the poster updates without issue, the video editor modal closes and goes back to the Media Library modal, which will load the VideoPress player with the new poster showing.

In the event of an error occurring when updating a poster, we're not in a situation where the poster URL needs to be reverted because it was never changed in the first place.

As for the RESET_STATE action, that's there to clear out any existing state when opening the video editor modal again. Otherwise you might open it and see an error message if your first attempt at updating the poster wasn't successful.

We do actually surface an error message to the user

did I miss this? where does that happen?

dmsnell · 2017-03-23T04:13:47Z

client/state/data-layer/wpcom/videos/poster/index.js

+};
+
+export const updatePosterUrl = ( { dispatch }, action, next, { poster } ) => {
+	dispatch( setPosterUrl( poster ) );


I guess poster is the name of the value coming from the API? it might help clarity here to rename in the arguments because it feels like poster ought to be an object here instead of a URL string…

…, next, { poster: posterURL } ) => {

dmsnell · 2017-03-23T04:20:22Z

client/state/ui/editor/video-editor/reducer.js

+export const uploadProgress = ( state = 0, action ) => {
+	switch ( action.type ) {
+		case VIDEO_EDITOR_SHOW_UPLOAD_PROGRESS:
+			return action.percentage;


some advice I'd share from the importer is that in some cases 100% is a value we never really see and it might be beneficial to fib a bit here. since 100% came at the very end, or never came because the load event occurred after something close like 96% I made it look like 100% names at an early value and this meant that people would see a positive confirmation that the processes was ending vs going from processing to oops I missed the end.

this can also (maybe more appropriately) be handled in the UI

dmsnell · 2017-03-23T04:23:58Z

@donnapep I'm still a bit leery about whether or not we need specific ERROR actions or even the RESET action. I think you said more error handling was on its way later, so please disregard if I'm overlooking that.

on the reset, what is a reset other than setting the URL to '' or null? I think we can reuse our existing state if we think about the meaning of what we're storing. Often times I will go ahead and provide the action creator but reuse the existing action types. This helps keep our code bloat down and improves our reuse.

const setUrl = url => ( { type: SET_URL, url } );

const resetUrl = () => setUrl( '' );

const url = ( state = null, { type, url } ) =>
	SET_URL === type
		? url
		: state;

const percentage = ( state = null, { type, percentage, url } ) => {
	if ( UPDATE === type ) {
		return percentage;
	}

	if ( SET_URL === type && ! url ) {
		return 0;
	}

	return state;
}

After the poster is successfully updated, an action to set the poster URL is dispatched, followed by a separate action to indicate that the video editor modal should close. The `shouldCloseVideoEditorModal` selector replaces the `isPosterUpdated` selector, and uses better naming to more accurately reflect its purpose.

In particular, VIDEO_EDITOR_CLOSE_MODAL can be used to reset the upload progress so that if the modal is reopened, the UI is correct (i.e. no progress bar showing and buttons are enabled).

donnapep · 2017-03-28T11:28:04Z

@dmsnell I've made some changes to the Redux actions and was able to eliminate the VIDEO_EDITOR_RESET_STATE action in 7e22d20 as per your suggestion.

I did some other refactoring and, thanks to my amazing image annotation abilities, hereby present to you the following screen captures that hopefully help to better illustrate why each action is necessary and how it is used.

There are 5 actions added by the video editor, namely:

VIDEO_EDITOR_CLOSE_MODAL (closes the video editor modal after a successful update - added by f69706a)
VIDEO_EDITOR_SET_POSTER_URL (the poster URL is returned by the API and is used to update the thumbnail in the Media Library)
VIDEO_EDITOR_SHOW_ERROR (shows an error if the poster could not be updated)
VIDEO_EDITOR_SHOW_UPLOAD_PROGRESS (shows the progress bar while an image is being uploaded)
VIDEO_EDITOR_UPDATE_POSTER (calls the API to update the poster and return the poster URL)

dmsnell

@donnapep you did lots of work on this and the PR is in a very nice condition. I'm happy with you merging it in; very good work!

donnapep · 2017-04-03T11:03:33Z

@dmsnell 🎉 Once again, thank you for your infinite patience and wisdom!

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 28, 2017

donnapep assigned donnapep and unassigned donnapep Feb 28, 2017

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 28, 2017

donnapep requested a review from aduth February 28, 2017 13:47

donnapep added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 28, 2017

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 28, 2017

donnapep mentioned this pull request Mar 1, 2017

Video: Add support for selecting or uploading a custom image to serve as the video poster #11389

Closed

donnapep requested review from marcinbot, lamosty and gwwar March 6, 2017 16:46

donnapep self-assigned this Mar 8, 2017

This was referenced Mar 8, 2017

Video Editor: Add VideoPress poster picker #11653

Merged

Video Editor: Add new video-editor block to enable selecting a custom video poster #11648

Merged

gwwar reviewed Mar 8, 2017

View reviewed changes

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 9, 2017

dbtlr suggested changes Mar 9, 2017

View reviewed changes

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 10, 2017

donnapep commented Mar 10, 2017

View reviewed changes

dmsnell reviewed Mar 17, 2017

View reviewed changes

dbtlr approved these changes Mar 19, 2017

View reviewed changes

dmsnell requested changes Mar 19, 2017

View reviewed changes

donnapep added 5 commits March 21, 2017 14:50

Move selectors and tests to state/selectors

5e6d625

Update documentation comments in reducer

34d26bb

Refactor handler for updating the poster

e692e94

Refactor action creators to be more concise

c192bf8

Use 'atTime' instead of 'at_time'

e1a0606

dmsnell reviewed Mar 23, 2017

View reviewed changes

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 24, 2017

donnapep added 4 commits March 27, 2017 09:32

Minor tweaks to video editor poster handler

ef2706f

Rename hasPosterUpdateError selector to shouldShowVideoEditorError

425af57

Eliminate VIDEO_EDITOR_RESET_STATE action

7e22d20

In particular, VIDEO_EDITOR_CLOSE_MODAL can be used to reset the upload progress so that if the modal is reopened, the UI is correct (i.e. no progress bar showing and buttons are enabled).

dmsnell approved these changes Apr 1, 2017

View reviewed changes

donnapep merged commit 2f51cbc into master Apr 3, 2017

donnapep deleted the add/video-editor-redux-state branch April 3, 2017 11:04

donnapep removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 3, 2017

Video: Redux state changes for custom video poster #11633

Video: Redux state changes for custom video poster #11633

Conversation

donnapep commented Feb 28, 2017 • edited by designsimply

matticbot commented Feb 28, 2017

donnapep commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar Mar 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbtlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnapep Mar 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnapep Mar 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderkevin Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Mar 10, 2017

donnapep commented Mar 13, 2017 • edited

dmsnell commented Mar 13, 2017

donnapep commented Mar 14, 2017 • edited

dmsnell commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

donnapep commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Mar 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Mar 23, 2017 • edited

donnapep commented Mar 28, 2017 • edited

dmsnell left a comment

Choose a reason for hiding this comment

donnapep commented Apr 3, 2017

donnapep commented Feb 28, 2017 •

edited by designsimply

gwwar Mar 8, 2017 •

edited

donnapep Mar 9, 2017 •

edited

donnapep Mar 9, 2017 •

edited

coderkevin Mar 10, 2017 •

edited

donnapep commented Mar 13, 2017 •

edited

donnapep commented Mar 14, 2017 •

edited

dmsnell Mar 23, 2017 •

edited

dmsnell commented Mar 23, 2017 •

edited

donnapep commented Mar 28, 2017 •

edited