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

Create TinyMCE command for adding media #5092

Merged
merged 2 commits into from
Apr 29, 2016
Merged

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Apr 28, 2016

The "add media" dialog appears after clicking on the "add media" icon in
the TinyMCE toolbar.

Previously, this occurred as a direct result of the onclick handler in
editor.addButton(). Because this meant that adding media was dependent
on clicking the button, it prevented being able to do so in other ways,
such as is the goal in #5071

This patch moves the code to cause the media dialog to appear into a new
command, which the button then calls when it's clicked.

Testing

There are no visual or functional changes here. Try to edit a post and add media. Click on the "add media" button. Test any other existing ways you know of to add media into a post to make sure this doesn't introduce any regressions.

Questions

  • Is it wrong to name the action the same thing as the button? Do they live in different namespaces or will we inadvertently cause collisions by doing this? - I have renamed the command based on a suggestion from @rodrigoi - it is now wpcomAddMedia

cc: @aduth

The "add media" dialog appears after clicking on the "add media" icon in
the TinyMCE toolbar.

Previously, this occurred as a direct result of the `onclick` handler in
`editor.addButton()`. Because this meant that adding media was dependent
on clicking the button, it prevented being able to do so in other ways,
such as is the goal in #5071

This patch moves the code to cause the media dialog to appear into a new
command, which the button then calls when it's clicked.
@dmsnell dmsnell added [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 Apr 28, 2016
@rodrigoi
Copy link
Contributor

Code looks good and I didn't find any regressions.

There is nothing on TinyMCE's documentation warning about collisions between buttons and commands. I don't think it will be an issue.
What could be an issue is that we have two different styles when naming commands and buttons. I personally like wpcomAddMedia for commands and wpcom_add_media for buttons. It may be a good idea to choose a style, considering that we'll be adding many commands and buttons to the widget drop down in the near future. Thoughts @aduth ?

@dmsnell
Copy link
Member Author

dmsnell commented Apr 28, 2016

I personally like wpcomAddMedia for commands and wpcom_add_media for buttons.

Done! thanks for the suggestion

@@ -281,28 +281,28 @@ function mediaButton( editor ) {
renderDropZone( { visible: event.type === 'dragend' } );
}

editor.addCommand( 'wpcomAddMedia', () => {
var selectedSite = sites.getSelectedSite();
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for the var over a const?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gwwar just because I was trying not to change the code

you think I should change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's already mixed usage in the file, so either way works with me. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be fine to change I think.

@gwwar
Copy link
Contributor

gwwar commented Apr 28, 2016

Is this supposed to be behind a feature flag?

I see the new edit option
screen shot 2016-04-28 at 4 47 27 pm
and a temp modal
screen shot 2016-04-28 at 4 46 43 pm

@dmsnell
Copy link
Member Author

dmsnell commented Apr 29, 2016

Is this supposed to be behind a feature flag?

Strange @gwwar - I see this behavior in master

@aduth - any thoughts?

@aduth
Copy link
Contributor

aduth commented Apr 29, 2016

I see the new edit option

@gwwar @dmsnell This is an in-progress feature related to #307, feature-flagged by post-editor/media-advanced (development environments only).

@aduth
Copy link
Contributor

aduth commented Apr 29, 2016

Looks good 👍

@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 Apr 29, 2016
@dmsnell dmsnell merged commit 22bdc61 into master Apr 29, 2016
@dmsnell dmsnell deleted the update/add-media-command branch April 29, 2016 16:57
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants