WIP: Add "insert items" menu item to editor #5071

dmsnell · 2016-04-28T05:08:30Z

Part of #5070

Adds an "insert item" menu bar to the editor toolbar to allow inserting various types of media or embeds into a post, similar to how the insert menus in mainstream applications work.

This PR's goal is to add in the empty shell on the editor toolbar which will make it easy to add other modules or embeds.

Testing

Load the editor
Check for the new dropdown on the top left of the toolbar

Questions

There's something pretty strange about the way this code replaces the top HTML in the split button. It's deeply coupled into the DOM structure of what the TinyMCE menu creates. I would appreciate some advice on a better or more idiomatic way to do this.
The toolbar button initialization and runtime code is tightly coupled to TinyMCE's API. This is making it hard to abstract the buttons and actions; for example, it's not possible to simply add the existing media plugin into the new dropdown menu. Our options are to refactor the existing TinyMCE plugins or to provide an adapter interface to mock the TinyMCE API and pass through the appropriate calls to the actual TinyMCE. None of this feels like the "right way to do it" so if anyone can offer some wisdom that would help us keep the code clean. As long as the other plugins create a new TinyMCE command, we can use that command in the insert menu without having to rewire anything else.

cc: @dan @aduth @drw158 @rodrigoi

mtias · 2016-04-28T11:22:12Z

client/components/tinymce/plugins/insert-menu/style.scss

+.mce-container.mce-menu .mce-container-body .mce-wpcom-insert-menu__menu-item {
+	margin-left: -6px;
+
+	& svg {


Could we use classes to target these?

Also, prefixing descendent selector with & is redundant and can be removed safely.

Thanks @mtias and @aduth. This is still very much a work-in-progress. I have rearranged the CSS to make better use of classes for specifying the styles, but I'm still pretty troubled by the strange .mce classes.

I'm new to TinyMCE work and had trouble getting enough specificity to overwrite the TinyMCE-provided styles. Any help to clean this up would be much-appreciated!

Yes, it'd be nice to replace the whole toolbar with our components and scope at some point.

aduth · 2016-04-28T15:06:42Z

We should feature flag this plugin until it's fully implemented.

dmsnell · 2016-04-28T18:25:56Z

We should feature flag this plugin until it's fully implemented.

Will do soon @aduth but not yet. I think this is going to involve some bigger changes and I think that I would prefer for now to get a little further along before thinking of merging. In other words, I want to see how far this reaches before I send out twenty different if ( config.isEnabled( '…' ) ) checks

dmsnell · 2016-04-28T18:28:39Z

@drw158 if you can take a look at this I would appreciate the CSS review. I have diverged from the example in the picture above by turning the color of the menu items blue when hovered instead of changing the background color to blue. This is what the format selector to the immediate right of the control does, so I chose it for consistency.

Styling questions

Hover emphasis - should it be background-color or color?
CSS mess - is it sane styling?
Drop-down chevron - I couldn't get this thing to look right on hover. There's a box that appears over the whole button and it cuts off to the left of the chevron.
I left the add media button as a separate button instead of the default. After looking at some apps that we cited in the issue, such as Pages, and getting some good feedback from a friend, I realized that those apps don't have a default click-action on the "insert dropdowns." They always dropdown. This is the behavior I replicated here but am willing to change.

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.

davewhitley · 2016-04-28T20:50:00Z

What you decided for the hover state is good — just text color. Looks like Calypso dropdowns are standard in that way.

I think the CSS looks good. The selectors are super specific, but that might be a tinyMCE thing.

I added some right padding on the button, and I think it's behaving correctly now.

.mce-toolbar .mce-btn-group .mce-btn.mce-insert-menu button {
    padding-right: 20px;
}

Normally we wouldn't use so many selectors, but I just copied the CSS for the text style dropdown.

I also got rid of a & that was not needed.

I left the add media button as a separate button instead of the default.

We might change this behavior later, but we want to combine the Add Media button and this dropdown. Most apps have a single "Add" button that encapsulates everything, so every action takes 2 clicks. But, we want to keep the Add Media selectable with just one click. Similar to something like this:

Might be helpful, but I noticed that the text color selection dropdown in the Editor, uses 2 buttons — one for the icon, and another for the dropdown caret.

dmsnell · 2016-04-28T22:03:06Z

We might change this behavior later, but we want to combine the Add Media button and this dropdown. Most apps have a single "Add" button that encapsulates everything, so every action takes 2 clicks.

Thanks @drw158 - I will change it back to the plan. The only reason I diverged was because in practice it felt confusing the way it did one thing if you click on the icon and another thing if you click on the chevron. Some of our justification for doing it this was (based on the behavior of other apps) was actually reported incorrectly in the earlier discussions too.

dmsnell · 2016-04-28T22:03:38Z

Might be helpful, but I noticed that the text color selection dropdown in the Editor, uses 2 buttons — one for the icon, and another for the dropdown caret.

I'll probably leave this as-is for now because of the TinyMCE wrangling. We should be able to supply the appropriate styling to do what we need visually.

dmsnell · 2016-04-28T22:04:26Z

The selectors are super specific, but that might be a tinyMCE thing.

I'm very open for help here and think it's ugly as-is, but I was really struggling to get a higher specificity than the styles from TinyMCE that were overwriting my changes.

dmsnell · 2016-04-28T22:14:01Z

@drw158 I switched back to the split button, and I am sorry to say that I broke your lovely box-padding fix around the chevron. Would you be willing to take another look at it?

davewhitley · 2016-04-29T15:09:25Z

The only reason I diverged was because in practice it felt confusing the way it did one thing if you click on the icon and another thing if you click on the chevron.

This may prove to be problematic, but should come up in testing. It's possible users won't even notice that you can click on the chevron to get the dropdown menu.

Create TinyMCE command for adding media 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.

davewhitley · 2016-04-29T20:49:09Z

I think the design is in an acceptable state now.

I'm not confident in the CSS; there are a lot of classes that I'm unsure about. It works though.

dmsnell · 2016-04-30T00:22:57Z

@drw158 I don't want to rain on your parade, but I noticed that when I hover over the label for one of the items in that dropdown menu, the icon doesn't highlight. The same is true if I hover over the icon, where the label doesn't highlight.

davewhitley · 2016-05-02T17:11:52Z

Np, I just pushed a fix.

davewhitley · 2016-05-02T22:30:56Z

I'm concerned about:

If users will know to click the dropdown
Users won't know where the contact form went

But, I don't think this small of a change really warrants a Horizon test. Can we deploy on wpcalypso first? We can feedback internally.

dmsnell · 2016-05-02T23:16:21Z

@drw158 and @aduth I have hidden this behind a config flag, but the code actually still loads on every request. Personally I don't feel much hesitation to do this since I haven't found any bugs or errors the code produces, but if you think it's worth it we can hide it all. Each step towards preventing it from loading at all in the production environment simply requires more and more if ( config.isEnabled( … ) ) checks and I'm trying to avoid that.

aduth · 2016-05-03T12:34:35Z

client/components/tinymce/plugins/insert-menu/menu-items.jsx

+	{
+		name: 'insert_media_item',
+		icon: 'add-image',
+		item: <GridiconButton icon="image-multiple" label={ 'Add Media' } />,


These labels should be translated?

Thanks @aduth - translations added in d5419ce

Note that we need a string review of Insert content because it's currently untranslated. We should review the wording before merging and having people translate that particular message.

davewhitley · 2016-05-20T20:05:27Z

Did the large icon get fixed?

aduth · 2016-05-20T20:12:54Z

Did the large icon get fixed?

I don't see any changes that would indicate that it has been, but I haven't spun up the branch to verify.

dmsnell · 2016-05-20T20:20:02Z

Did the large icon get fixed?

haha, whoops I didn't test it and had assumed you got it @drw158

Adds a basic shell for a new split button in TinyMCE, meant to be pluggable to display a variety of menu options to insert embeds/widgets.

This patch changes the behavior of the insert menu dropdown, whereas previously we had a separate add media button and then the insert button, which always opened the drop-down. Now, we have a single button that will add media on a direct click and only open the drop-down when the chevron is clicked.

Add translation to strings

Changing color of caret on hover

@aduth

Props to @aduth for the suggestions

dmsnell · 2016-05-24T19:31:34Z

The icon size should be fixed thanks to @drw158's suggestions.

Before

After

Tested in IE11 and verified in Chrome that it didn't mess up.

@aduth

Fix of #5071 - was missing a config flag check Adds an "insert item" menu bar to the editor toolbar to allow inserting various types of media or embeds into a post, similar to how the insert menus in other mainstream applications (such as Pages, Sketch, etc…) work. This patch removes the contact form button and the media button and combines those into the new insert menu. The insert menu button has two behaviors: a default click on the button inserts a media item; a click on the chevron opens the drop-down menu to select one of many possible items to insert. Props to all who helped review and edit and design this feature: @aduth @drw158 @folletto @mtias @rralian

dmsnell added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] In Progress labels Apr 28, 2016

mtias reviewed Apr 28, 2016
View reviewed changes

dmsnell force-pushed the add/editor-insert-plugin branch from 072c3ca to a660563 Compare April 28, 2016 18:50

dmsnell mentioned this pull request Apr 28, 2016

Create TinyMCE command for adding media #5092

Merged

1 task

dmsnell force-pushed the add/editor-insert-plugin branch from f095f0e to 506668d Compare April 29, 2016 17:16

dmsnell 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 May 2, 2016

dmsnell force-pushed the add/editor-insert-plugin branch from 061e03b to 4243e3a Compare May 2, 2016 22:48

rralian added this to the Plans: Iteration 2 milestone May 3, 2016

aduth reviewed May 3, 2016
View reviewed changes

dmsnell and others added 15 commits May 24, 2016 14:40

Add experimental toolbar dropdown shell

7276697

Adds a basic shell for a new split button in TinyMCE, meant to be pluggable to display a variety of menu options to insert embeds/widgets.

Correcting button layout and design

4b50778

Adjusting layout of menu

2ddd093

Making sure that hover changes the element color

826cb67

Add translation to strings

Simplify rendering of splitbutton React component

22cdc95

Editor: Adjust insert content button border

280eff5

Increasing the size of the button

67b0f6f

Changing color of caret on hover

Fixing border left color

a703104

CSS cleanup

5dc34c3

alternating colors of icons on hover

bac3142

Enable in Horizon

69c6af8

making the hover color change less dramatic

218615f

Hide editor init behind feature flag, cleanup

bb15905

Props to @aduth for the suggestions

Merge fix

adaa14b

dmsnell force-pushed the add/editor-insert-plugin branch from c1ec490 to adaa14b Compare May 24, 2016 18:40

Fix IE 11 icon size rendering issue

267f722

davewhitley 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 May 24, 2016

dmsnell merged commit 65c22ab into master May 24, 2016

dmsnell deleted the add/editor-insert-plugin branch May 24, 2016 19:59

matticbot removed the [Status] Ready to Merge label May 24, 2016

dmsnell mentioned this pull request May 24, 2016

Revert "WIP: Add "insert items" menu item to editor" #5568

Merged

dmsnell restored the add/editor-insert-plugin branch May 24, 2016 20:49

dmsnell mentioned this pull request May 25, 2016

Add editor insert plugin #5584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add "insert items" menu item to editor #5071

WIP: Add "insert items" menu item to editor #5071

dmsnell commented Apr 28, 2016 •

edited

Loading

mtias Apr 28, 2016

aduth Apr 28, 2016

dmsnell Apr 28, 2016

mtias Apr 28, 2016

aduth commented Apr 28, 2016

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016 •

edited by davewhitley

Loading

davewhitley commented Apr 28, 2016 •

edited

Loading

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016

davewhitley commented Apr 29, 2016

davewhitley commented Apr 29, 2016

dmsnell commented Apr 30, 2016

davewhitley commented May 2, 2016

davewhitley commented May 2, 2016

dmsnell commented May 2, 2016

aduth May 3, 2016

dmsnell May 4, 2016

davewhitley commented May 20, 2016

aduth commented May 20, 2016

dmsnell commented May 20, 2016

dmsnell commented May 24, 2016

WIP: Add "insert items" menu item to editor #5071

WIP: Add "insert items" menu item to editor #5071

Conversation

dmsnell commented Apr 28, 2016 • edited Loading

mtias Apr 28, 2016

Choose a reason for hiding this comment

aduth Apr 28, 2016

Choose a reason for hiding this comment

dmsnell Apr 28, 2016

Choose a reason for hiding this comment

mtias Apr 28, 2016

Choose a reason for hiding this comment

aduth commented Apr 28, 2016

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016 • edited by davewhitley Loading

davewhitley commented Apr 28, 2016 • edited Loading

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016

dmsnell commented Apr 28, 2016

davewhitley commented Apr 29, 2016

davewhitley commented Apr 29, 2016

dmsnell commented Apr 30, 2016

davewhitley commented May 2, 2016

davewhitley commented May 2, 2016

dmsnell commented May 2, 2016

aduth May 3, 2016

Choose a reason for hiding this comment

dmsnell May 4, 2016

Choose a reason for hiding this comment

davewhitley commented May 20, 2016

aduth commented May 20, 2016

dmsnell commented May 20, 2016

dmsnell commented May 24, 2016

dmsnell commented Apr 28, 2016 •

edited

Loading

dmsnell commented Apr 28, 2016 •

edited by davewhitley

Loading

davewhitley commented Apr 28, 2016 •

edited

Loading