Media: add media section #11822

retrofox · 2017-03-07T09:17:13Z

This PR makes a number of improvements to the media section and updates the media modal to include an action bar at the top. The media section is controlled by the manage/media feature flag. This is currently enabled in dev, wpcalypso, horizon and test. The media section can be shown by clicking on the Media option into the sidebar.

Related Design Discussion: #10805

The whole picture

Primary Changes

Beyond add the media library this PR implements many changes. Following, the most important ones:

Media -> Add item

It allows adding a new item quickly.

Media -> Actions bar

Media -> Plan storage nudge

A plan storage nudge is shown in the filter bar is the site doesn't have a business plan.

Media -> Item detail dialog

Added DONE and DELETE buttons

Post editor -> add media -> Item detail dialog

Actions buttons were removed from the bottom
Added the actions bar above the media gallery

Testing Instructions:

Media Modal

Navigate to http://calypso.localhost:3000/post/
Select a site when prompted
Click on insert content:

The new action bar appears at the top.
The plan storage component in the top right corner can be seen for folks with a wpcom free/personal/premium plan.

Clicking "Add new" opens the file picker

Click on the chevron next to "add new" gives you this option:

Clicking on Add Via Url updates the interface:

Selecting items adds an edit and delete button to the action bar

Search works as expected
Other media modal flows work as expected

Media Section

Click on the add button of the media section

The media section should open with the file picker ready
Clicking on the section link outside of "add" should just open the media section
The media section visually looks ok

We can edit media like we can do in the media modal
scrolling down, stickies the action bar:

Site Icon

We can update a public site icon correctly
Navigate to http://calypso.localhost:3000/settings/general/
Select a public site
Click on "Change" underneath the site icon
Follow flow, site icon should update

Known Visual Issues

There are a few known visual issues in the media section that should be addressed after this feature branch lands. It should be safe to land with these quirks since the media section is not enabled in prod yet. We're hoping to land the feature branch quickly to avoid drift and a larger diff.

Image Editor in Media Section shows too many buttons:
Visual quirks at smaller sizes with the sticky bar

Pull Requests

This process was done working in different patches:

Media Section: Move Action buttons to top, multiple selection Media section: Move Action buttons to top, multiple selection #11589
Media Section: Add Link Media section/add link #11672
Media Section: Action bar buttons + card Media section: Action bar buttons + card #11560
Media Section: Move Action buttons to top, multiple selection Media section: Move Action buttons to top, multiple selection #11589
Media Section: tidying secondary actions Media section: tiding secondary actions #11770
Component: reimplement plan-storage structure using flexbox Component: reimplement plan-storage structure using flexbox #11769
Media: add plan-storage component Media: add plan-storage component #11749
Plan Storage: Minor style adjustments Plan Storage: Minor style adjustments #11858
Media Library: fix elevation visual bug in search icon Media Library: fix elevation visual bug in search icon #11836
Media: fix-plan storage for business plan Media: fix-plan storage for business plan #11870
Media modal: do not apply custom styles to uploading-by-URL box Media modal: do not apply custom styles to uploading-by-URL box #11932
Media: add Done and Delete buttons Media: add Done and Delete buttons #11910
Media Library: upload-url janitorial Media Library: upload-url janitorial #11828
Media Library: improve uploading by URL section Media Library: improve uploading by URL section #11830
Media Section: Style cleanup for action bar Media Section: Style cleanup for action bar #11873

h/t to @iamtakashi @artpi @retrofox @gwwar

matticbot · 2017-03-07T09:17:14Z

Test live: https://calypso.live/media?branch=try/media-section-v2

gwwar · 2017-03-09T19:58:57Z

cc @alisterscott @hoverduck would it be possible to run e2e's against this branch? Let us know if we need to update any test suites.

hoverduck · 2017-03-09T21:29:16Z

@gwwar The e2e tests look good except (unsurprisingly) the media upload test needs some updates. Since you're the original author can you take a look? If not I can get to it tomorrow

dmsnell · 2017-03-09T21:40:47Z

client/my-sites/media-library/filter-bar.jsx

+		const eventProperties = { cta_name: 'plan-media-storage' };
+		return (
+			<PlanStorage siteId={ this.props.site.ID }>
+				<TrackComponentView eventName={ eventName } eventProperties={ eventProperties } />


fun fact! when I'm just passing named properties like this I find the spread syntax helpful…

<TrackComponentView { ...{ eventName, eventProperties } } />

gwwar · 2017-03-09T21:47:26Z

Thanks @hoverduck I'll try to take a look at that today

klimeryk · 2017-03-09T22:20:46Z

client/my-sites/media-library/upload-url.jsx


 		if ( isError || ! this.props.site ) {
+			event.preventDefault();


Just a quick drive-by comment - it seems we need to call event.preventDefault() in both cases, so it should probably be called once (before this if branch and removed from line 45) - otherwise someone might think that one of these calls should not be there :)

I'm sorry Igor, I don't understand completely what do you mean. ~~Could you point straight in the file of this PR?~~ I'm sorry. It seemed to be a connection issue since I couldn't see this comment here.

Now I got it. You're right. Thanks.

Ah, sorry. So, no matter which flow we take in this function we end up calling event.preventDefault. Either here: https://github.com/Automattic/wp-calypso/pull/11822/files#diff-246b58e6f07d7dce525ca439d82eccf9R41 or here: https://github.com/Automattic/wp-calypso/pull/11822/files#diff-246b58e6f07d7dce525ca439d82eccf9R45.
So I thought that this call should be "deduplicated" and moved to a single call, somewhere above https://github.com/Automattic/wp-calypso/pull/11822/files#diff-246b58e6f07d7dce525ca439d82eccf9R40. Less chances of one of these calls getting deleted and it shows the intent (always prevent default behaviour of the event) better.

Actually, this line seems to be unneeded. Good one 👍

klimeryk · 2017-03-09T22:28:26Z

client/my-sites/media/main.jsx

+
+		accept( confirmMessage, accepted => {
+			if ( ! accepted ) {
+				return null;


Seems like just return; should suffice :)

Yes, agree. But I've gotten feedback from other PR that we should be explicit in this sense. Although return returns a null if nothing is defined, it's more understandable when the null value is defined explicitly.

Ah, got it - I was thrown off by no explicit return in the success scenario, but if that's intended then cool 👍

yes ... we aren't being consistent with this. :-|

Although return returns a null if nothing is defined, it's more understandable when the null value is defined explicitly.

interesting…what reason was given to be explicit in the return value there?

also this is wrong. return returns undefined if no value is given.

also this is wrong. return returns undefined if no value is given.

You're right. Another reason to return null ;-)

You're right. Another reason to return null ;-)

the way I typically do it is that if my only reason to use return is to early-abort from a function then I'll just return because that's how it would have happened if the function had returned normally given no explicit value.

if, on the other hand, the function is designed to return a value and something else is counting on it I'll want to explicitly return something. bonus points if it's the same data type as would otherwise return.

for example, an empty string could be more appropriate for a string function than null, a 0 might be more appropriate for a number function, an empty list for a list function, etc…

but if I'm calling a function for its side-effects only and don't expect a value, I think getting something back could also be confusing

But I've gotten feedback from other PR that we should be explicit in this sense. Although return returns a null if nothing is defined, it's more understandable when the null value is defined explicitly.

Do you have the link @retrofox? We might have gotten confused that this was a render helper. A return null for rending is an explicit indication for React that we want to render nothing.

I missed this on the original PR, but it doesn't look like anything uses this return value, in which case we should just return; I agree with @dmsnell on consistency. A function should either always return some value if we're using it, or nothing if we don't.

Yes, sorry. Last night I forgot to leave my answer here.
I was confusing with the render method of React component. Let me update the code. I'm sorry again and thanks!

rebased and updated

alisterscott · 2017-03-09T22:34:27Z

Thanks @hoverduck I'll try to take a look at that today

@gwwar: if you don't find time please LMK and I can do this for you

…section

gwwar · 2017-03-10T16:21:02Z

Thanks @folletto for testing!

(1) Media -> Actions bar Would be ideal if the popup text wasn't small and uppercase, but normal and with a standard font

@retrofox let's update styling for this

(2) Media -> Plan storage nudge I'd leave the final decision to @iamtakashi, but I think the whole area should be clickable, not just "Upgrade" (3) Media -> Item detail dialog I'd also add a "Cancel" / "Close" button.

I think we can follow up pretty quickly and discuss points 2 and 3 after the feature branch lands.

retrofox · 2017-03-10T16:24:29Z

(1) Media -> Actions bar Would be ideal if the popup text wasn't small and uppercase, but normal and with a standard font

@retrofox let's update styling for this

sure.

Thanks @folletto for the feedback. Let's create and add issues for (2) and (3) in our backlog as well.

retrofox · 2017-03-10T16:38:46Z

f5061c2

cc @folletto , @gwwar

folletto · 2017-03-10T16:43:28Z

It's a nitpick, but i'd make the clickable item 33px tall too. :)

retrofox · 2017-03-10T16:51:51Z

It's a nitpick

nop, it isn't.

but i'd make the clickable item 33px tall too. :)

Sorry, do you mean increase from 26px to 33px there?

folletto · 2017-03-10T17:10:45Z

Sorry, do you mean increase from 26px to 33px there?

Yep. The blue area basically.

retrofox · 2017-03-10T17:13:47Z

Sorry, do you mean increase from 26px to 33px there?

Yep. The blue area basically.

yes, already done. I've removed the is-compact className from the PopoverMenuItem component.

retrofox added [Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Status] In Progress labels Mar 7, 2017

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

retrofox force-pushed the try/media-section-v2 branch from 66fce78 to 2474f28 Compare March 8, 2017 17:02

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

retrofox force-pushed the try/media-section-v2 branch from 5d54642 to 98cdd8e Compare March 8, 2017 21:34

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

retrofox force-pushed the try/media-section-v2 branch from 9ae39f5 to 2e280ee Compare March 9, 2017 18:55

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

retrofox changed the title ~~[wip] Try/media section v2~~ Media: add media section Mar 9, 2017

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

retrofox force-pushed the try/media-section-v2 branch from c735f39 to e6e6b0b Compare March 9, 2017 20:08

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

gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 9, 2017

gwwar requested review from lamosty, artpi, rralian, aduth, dmsnell and folletto March 9, 2017 21:02

dmsnell reviewed Mar 9, 2017

View reviewed changes

klimeryk reviewed Mar 9, 2017

View reviewed changes

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

retrofox and others added 9 commits March 10, 2017 13:14

media: adjust media modal buttons

fcf378c

media-library: add className property

8a9a8d4

media: add custom className to media library

3aa8bf7

media: apply custom styles to upload-URL box only in this main media …

08aeab0

…section

Media Section: Style cleanup for action bar

668b683

Media Section/modal: Position adjustments

f2461b7

Media Section/Modal: Remove an empty line

0cf1c0c

Media Section/Modal: Avoid a jum when add via url field pops up

08a6a6d

Media: fix search z-index over sticky bar

eefefd6

retrofox force-pushed the try/media-section-v2 branch from f1f89b0 to eefefd6 Compare March 10, 2017 16:25

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

retrofox force-pushed the try/media-section-v2 branch from f5061c2 to fbb8ffc Compare March 10, 2017 16:54

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

media-library: apply custom style to Add-via-URL

3d16a80

retrofox force-pushed the try/media-section-v2 branch from fbb8ffc to 3d16a80 Compare March 10, 2017 17:07

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

gwwar approved these changes Mar 10, 2017

View reviewed changes

gwwar merged commit 0ef16de into master Mar 10, 2017

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

This was referenced Mar 12, 2017

Media: Remove "<- Media Library" link from image detail modal. #12043

Closed

Plan Storage: Consider making the whole component clickable #12044

Closed

gwwar mentioned this pull request Mar 14, 2017

Media: enable media section in all environments #12122

Merged

gwwar deleted the try/media-section-v2 branch March 14, 2017 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media: add media section #11822

Media: add media section #11822

retrofox commented Mar 7, 2017 •

edited by gwwar

Loading

matticbot commented Mar 7, 2017 •

edited by folletto

Loading

gwwar commented Mar 9, 2017

hoverduck commented Mar 9, 2017

dmsnell Mar 9, 2017

gwwar commented Mar 9, 2017

klimeryk Mar 9, 2017 •

edited

Loading

retrofox Mar 9, 2017 •

edited

Loading

retrofox Mar 9, 2017

klimeryk Mar 9, 2017

retrofox Mar 9, 2017

klimeryk Mar 9, 2017

retrofox Mar 9, 2017

klimeryk Mar 9, 2017

retrofox Mar 9, 2017 •

edited

Loading

dmsnell Mar 9, 2017

retrofox Mar 9, 2017

dmsnell Mar 9, 2017

gwwar Mar 10, 2017

retrofox Mar 10, 2017

retrofox Mar 10, 2017

alisterscott commented Mar 9, 2017

gwwar commented Mar 10, 2017 •

edited

Loading

retrofox commented Mar 10, 2017

retrofox commented Mar 10, 2017 •

edited

Loading

folletto commented Mar 10, 2017

retrofox commented Mar 10, 2017 •

edited

Loading

folletto commented Mar 10, 2017

retrofox commented Mar 10, 2017 •

edited

Loading


		if ( isError \|\| ! this.props.site ) {
		event.preventDefault();

Media: add media section #11822

Media: add media section #11822

Conversation

retrofox commented Mar 7, 2017 • edited by gwwar Loading

The whole picture

Primary Changes

Media -> Add item

Media -> Actions bar

Media -> Plan storage nudge

Media -> Item detail dialog

Post editor -> add media -> Item detail dialog

Testing Instructions:

Media Modal

Media Section

Site Icon

Known Visual Issues

Pull Requests

matticbot commented Mar 7, 2017 • edited by folletto Loading

gwwar commented Mar 9, 2017

hoverduck commented Mar 9, 2017

Choose a reason for hiding this comment

gwwar commented Mar 9, 2017

klimeryk Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

retrofox Mar 9, 2017 • edited Loading

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

retrofox Mar 9, 2017 • edited Loading

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

alisterscott commented Mar 9, 2017

gwwar commented Mar 10, 2017 • edited Loading

retrofox commented Mar 10, 2017

retrofox commented Mar 10, 2017 • edited Loading

folletto commented Mar 10, 2017

retrofox commented Mar 10, 2017 • edited Loading

folletto commented Mar 10, 2017

retrofox commented Mar 10, 2017 • edited Loading

retrofox commented Mar 7, 2017 •

edited by gwwar

Loading

matticbot commented Mar 7, 2017 •

edited by folletto

Loading

klimeryk Mar 9, 2017 •

edited

Loading

retrofox Mar 9, 2017 •

edited

Loading

retrofox Mar 9, 2017 •

edited

Loading

gwwar commented Mar 10, 2017 •

edited

Loading

retrofox commented Mar 10, 2017 •

edited

Loading

retrofox commented Mar 10, 2017 •

edited

Loading

retrofox commented Mar 10, 2017 •

edited

Loading