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: Populate advanced editing with parsed media #2541

Merged
merged 3 commits into from
Feb 4, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jan 18, 2016

Related: #307, #2085

This pull request seeks to further progress advanced image editing by parsing the selected media item and adding the necessary behavior to update markup in the post editor after the user has chosen to save their changes.

WIP

This is very much a work-in-progress. As of now, the only expected behaviors are:

  • A user can edit a selected image from the inline toolbar
  • A modal appears, displaying the media's ID and a save button
  • Upon clicking Save, the image is replaced with the text "Updated"

Note that this feature is feature-flagged, and is only available in a development environment. You can/should verify that these options are unavailable in all other environments.

Implementation notes:

The advanced media modal is no longer rendered by the <PostEditor /> component, but instead by the media/advanced TinyMCE plugin. This is done so that the expected insertMedia prop handler can be passed to the modal which, when called, replaces the current selection with the passed markup.

Testing instructions:

  1. Navigate to the Calypso post editor
  2. Select a site, if prompted
  3. Insert an image into the post contents
  4. Select the image by clicking it
  5. Click Edit in the inline toolbar
  6. Note that a dialog is shown, containing the image's ID and a Save button
  7. Click Save
  8. Note that the modal is dismissed and that the image has been replaced in post contents with "Updated"

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Media The media screen in Calypso, general media management, or integration with third party media. labels Jan 18, 2016
@aduth aduth self-assigned this Jan 18, 2016
getDefaultProps() {
return {
resetEditItem: () => {},
insertMedia: () => {}
Copy link
Member

Choose a reason for hiding this comment

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

I think it works well when we use noop from lodash (or any standard noop). I like the idea of being able to quickly and easily grep for these kinds of things, plus if we all use the same one, it's just a single closure/function being generated. obviously this isn't a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it works well when we use noop from lodash (or any standard noop).

Yeah, I don't feel strongly either way, though I find it's nice to limit the dependencies, and this should become a common enough pattern for an empty function that I don't think it really harms readability.

@dmsnell
Copy link
Member

dmsnell commented Jan 27, 2016

@aduth I hope you don't mind the hit-and-run here, but I saw your PR without any comments and poked my head in. didn't see any concerning issues with the code but I left a couple comments. thanks for the work - the media system is really great IMHO

@aduth
Copy link
Contributor Author

aduth commented Feb 2, 2016

I hope you don't mind the hit-and-run here, but I saw your PR without any comments and poked my head in.

I'm always appreciative of your reviews 😃 Apologies for the delay in response, as I've been out for the past two weeks.

thanks for the work - the media system is really great IMHO

Thanks for the kind words!

@aduth aduth force-pushed the update/media-advanced-attributes branch from e0abfba to 25f05a6 Compare February 2, 2016 21:49
@aduth
Copy link
Contributor Author

aduth commented Feb 2, 2016

Force pushed a rebase, including changing getButtons to getButtonSettings in 25f05a6.

Marking as ready to merge, though not planning to merge immediately.

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 2, 2016
@aduth aduth force-pushed the update/media-advanced-attributes branch from 25f05a6 to cb8f8aa Compare February 4, 2016 17:04
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. [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants