Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media section: Move Action buttons to top, multiple selection #11589

Merged
merged 5 commits into from
Feb 28, 2017

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Feb 24, 2017

This PR resolves #11512, #10851 and puts some work towards #10805

  • Enables selecting of multiple media items
  • Moves secondary action buttons to top action bar where they can be redesigned
  • Splits off following tasks:

The bar does not look how it should, continue in #10805 to make it look proper

Show me

zrzut ekranu 2017-02-24 o 12 16 11
zrzut ekranu 2017-02-24 o 12 15 45

CC @iamtakashi @rralian @retrofox @gwwar @lamosty

@artpi artpi added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 24, 2017
@artpi artpi self-assigned this Feb 24, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] L Large sized issue label Feb 24, 2017
@retrofox
Copy link
Contributor

Is the idea apply these changes also in the media-modal?

Anyway, we should one of them in the media-modal.

renderStorage={ false }
site={ this.props.site }
view={ 'LIST' }
/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A trivial detail, but sometimes you're closing the component instance on a new line and sometimes not.

@@ -28,7 +31,8 @@ export default React.createClass( {
getInitialState: function() {
return {
editedItem: null,
openedDetails: null,
currentDetail: null,
selectedImages: []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done a68b222

openDetailsModal( item ) {
this.setState( { openedDetails: item } );
openDetailsModalForASingleImage( image ) {
const selected = [ image ];
Copy link
Contributor

@retrofox retrofox Feb 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that not much is gained by creating a constant here.

this.setState( {
  currentDetail: 0,
  selectedImages: [ image ]
} );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done a68b222

@artpi
Copy link
Contributor Author

artpi commented Feb 24, 2017

Is the idea apply these changes also in the media-modal?

Yes, as in #10805

@gwwar
Copy link
Contributor

gwwar commented Feb 24, 2017

Since this is difficult to make small changes here without breaking the flow in editor, let's target try/media-section-v2 instead of master. We'll try to scope that feature branch to just the action bar changes and plan storage modifications. Ideally this feature branch should not live longer than a week or so.

@gwwar gwwar changed the base branch from master to try/media-section-v2 February 24, 2017 19:40
Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @artpi this looks like a good first step. Smoke tested functionality in both the modal and the section and things look good. We should be ok to land in the shared branch.

I did notice some small behavior differences that we can address in follow ups:

  • In the media section select a number of items, click edit, page to some position other than the first item. Click "<- Media Library". Click Edit again. We start at position 0. (The media modal in the post editor remembers which item you had open).
  • Navigate to the post editor. Open up the Media Modal. We see the selection from the media section.

@iamtakashi Did you intend for the detail/edit pages to be a modal in the media section? Or did we want those pieces to be inline? If it's the latter ping me and we can create more issues.

screen shot 2017-02-24 at 11 58 30 am

@iamtakashi
Copy link
Contributor

@iamtakashi Did you intend for the detail/edit pages to be a modal in the media section?

Yes. I think it should be the same as editing images from the modal.

@gwwar
Copy link
Contributor

gwwar commented Feb 27, 2017

Yes. I think it should be the same as editing images from the modal.

@iamtakashi To clarify the image editor will be a modal atm, but did we intend for things like the image title/details/url etc to be editable inline on a page view for the media section.

@artpi
Copy link
Contributor Author

artpi commented Feb 27, 2017

In the media section select a number of items, click edit, page to some position other than the first item. Click "<- Media Library". Click Edit again. We start at position 0. (The media modal in the post editor remembers which item you had open).

That should be resolved in #11592

@iamtakashi
Copy link
Contributor

@gwwar I didn't consider inline editing like that at this moment. I can see the idea could work, but we'll have many questions to make it happen. I'll keep that mind!

@gwwar
Copy link
Contributor

gwwar commented Feb 27, 2017

Thanks @iamtakashi 👍

@artpi artpi force-pushed the media-section/action-bar-edit-buttons branch from 1dd45d5 to 1796d15 Compare February 28, 2017 09:10
@matticbot matticbot added the [Size] L Large sized issue label Feb 28, 2017
@artpi artpi merged this pull request into try/media-section-v2 Feb 28, 2017
@artpi artpi deleted the media-section/action-bar-edit-buttons branch February 28, 2017 09:23
@matticbot matticbot removed 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
artpi added a commit that referenced this pull request Mar 1, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
@artpi artpi mentioned this pull request Mar 3, 2017
4 tasks
artpi added a commit that referenced this pull request Mar 6, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
retrofox pushed a commit that referenced this pull request Mar 8, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
retrofox pushed a commit that referenced this pull request Mar 8, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
retrofox pushed a commit that referenced this pull request Mar 9, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
@retrofox retrofox added the [Feature] Media The media screen in Calypso, general media management, or integration with third party media. label Mar 9, 2017
retrofox pushed a commit that referenced this pull request Mar 9, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
retrofox pushed a commit that referenced this pull request Mar 10, 2017
* Make multiple selection work + admin bar
* Fix single image edit
* Mae bulk delete work
* Reuse MediaModalSecondaryActions
* Review fixes
@matticbot
Copy link
Contributor

@artpi This PR needs a rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Size] L Large sized issue [Status] Needs Rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants