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

Adds new RFC detailing improvements to TinyMCE #16

Merged
merged 6 commits into from Aug 20, 2019

Conversation

@warrenbuckley
Copy link
Member

commented Aug 7, 2019

A final comment period has been applied to this RFC and it will be accepted on 16/08/2019


This RFC is to give specific details on how we plan to improve the existing implementation of the TinyMCE version 4 rich text editor.

Read the full RFC here

warrenbuckley added some commits Aug 7, 2019

@Shazwazza
Copy link
Member

left a comment

see comments

cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved
cms/0000-TinyMCE-RTE-Improvements.md Outdated Show resolved Hide resolved

warrenbuckley added some commits Aug 7, 2019

@warrenbuckley warrenbuckley marked this pull request as ready for review Aug 7, 2019

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

A final comment period has been applied to this RFC and it will be accepted on 16/08/2019

@warrenbuckley

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@sniffdk the original goal of avoiding using an iframe was believed to be a performance bottleneck.
Regardless if TinyMCE is in iframe or inline DOM mode, there was no difference in speed/performance.

So iframe mode should not be seen as a bad thing as it has no impact on rendering performance from our tests.

but subsequent requests regardless if TinyMCE uses an IFrame or not it is fractionally slower than rendering the editor and its contents than Trix

@sniffdk

This comment has been minimized.

Copy link

commented Aug 8, 2019

Arghh removed my comment as most of it were found in the other PR's after closer inspection 😅
I do see iframe mode as a bad thing, as it has always caused issues when used in any kind of grid/block editor where stuff can move around, which causes the iframes to reload when they are moved in the DOM.

But it seems we can select a non-iframe version in v8, so that's just great 👍 😃

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@sniffdk sure thing, but tiny does support non-iframes in 2 different modes. Currently one of those modes is already in v8 which is the distraction free mode. The other one is with a floating toolbar, we may investigate that sometime in the future but as it is now, you can use an iframe-less editor with distraction free mode inside things like the grid, etc...

@sniffdk

This comment has been minimized.

Copy link

commented Aug 8, 2019

Got it.
Do you believe this is the best possible solution for the editors or are these decisions purely from a development effort perspective?

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Were you able to read the result of the previous one?#15 (comment)

@Swimburger

This comment has been minimized.

Copy link

commented Aug 8, 2019

I like the suggested improvements, but where TinyMCE falls short for me today is that you can't enter the width/height attribute for the img element when inserting an image. Somehow it determines which width and height the image should have by itself and then use style="width:...;height:..." attribute instead which isn't a best practice.
This results in me having to copy the HTML in VS Code and manually fixing each image in HTML.

Ps: not sure if this is the RFC you were looking for, I'll remove it if irrelevant from this discussion.

@sniffdk

This comment has been minimized.

Copy link

commented Aug 8, 2019

Reread it to be sure. Just to summarize.

  • Test subjects liked Tiny v5 (which should be pretty much the same as v4) best when comparing to other editors, so no real reason to change editor after all.

  • There is nothing new of value in Tiny v5 compared to v4 (which is the version used in umbraco v8) except api changes, and it would cause breaking changes anyways.

  • Performance is no longer an issue, but some improvements will still be made

  • Pasting from other source like Word might be improved, as will moving block elements around.

Anything I missed?

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Swimburger thanks for the feedback. Currently with Umbraco when it inserts images it does so based on a size specified in the data type IIRC and then the image is requested as the resized version so that it's not loading some huge hi-resolution image. I do think it uses width/height attributes but would need to check. The interesting part of this though is that we are in charge of inserting these images so we can improve how it's done. What you you like to see happen when inserting an image from the media library, just the image source as a limited size or the full hi-res image as-is or other options?

@Swimburger

This comment has been minimized.

Copy link

commented Aug 8, 2019

@Shazwazza Uploading images and other media to Umbraco is a great experience.
It's only when inserting the images into the TinyMCE there's a field for the alt-attribute (awesome!), but not for width/height. Here's a sample of the blog post I'm working on in my Umbraco based blog:

<p><img style="width: 500px; height: 278.4810126582278px;"
        src="/media/1187/azure-devops-releases-23-create-release-details.png?width=500&amp;height=278.4810126582278"
        alt="Azure DevOps create release details" data-udi="umb://media/8e5b042036c84ab68d36a3fd00e22c6c" /></p>
<p><img style="width: 500px; height: 278.4810126582278px;"
        src="/media/1188/azure-devops-releases-24-release-results.png?width=500&amp;height=278.4810126582278"
        alt="Azure DevOps Release results" data-udi="umb://media/d7804adf6d2d473394f7b0de1b6b7dff" /></p>
<p><img style="width: 500px; height: 278.4810126582278px;"
        src="/media/1189/azure-devops-releases-25-release-test-results.png?width=500&amp;height=278.4810126582278"
        alt="Azure DevOps release result test details" data-udi="umb://media/84ca5347d19c48c987acc5755e4e4c97" /></p>

I manually edit the HTML to reach this desired HTML:

<img width="1264" height="704"
        src="/media/1189/azure-devops-releases-25-release-test-results.png"
        alt="Azure DevOps release result test details" data-udi="umb://media/84ca5347d19c48c987acc5755e4e4c97" />

Steps to get the desired results:

  1. Remove the query string parameters
  2. Remove the style attribute
  3. Browse to the image to find the original resolution
  4. Use width/height attribute to specify the original resolution

Some of this is my own preference which may not be relevant to the general Umbraco Content Editors, fe wanting to use the original resolution.
Though, width/height attribute is the HTML standard and should be used over the style attribute.
It would be great if the image insert experience would help us specify the width and height.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Swimburger sounds like something we can cater for! We'll need to investigate options but can update the RFC to include information about this.

@BatJan

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

I'm wondering if some research can be put in to figure out how we can add styles/formatting options like Nik describes here https://www.justnik.me/blog/umbraco-rte-styles-another-way but where it's possible to differentiate the configs for different RTE instances? Using Nik's current approach the config will apply to every configured RTE datatypes where styling/formatting options are available.

If using the traditional approach of adding styling will potentially add some often unwanted <span> elements when a style is being applied. Sometimes one really wants to ensure that the class is added directly to a <p> element for and not to a <span>. There is a use case for the current approach so I don't think that should be changed - It would just be nice if it was somehow possible to use Nik's approach and target certain configured RTE's - Does my thinking make any sense? 😄

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Agreed with Jan, that one is an often requested feature. In general most people would like to be able to configure each Tiny instance with any of the configurations that Tiny provides. Maybe this could even be a textarea on the datatype configuration where you provide an exact, standard, Tiny config format.

Right now we have tinyMceConfig.config which is doing things a bit differently from the default Tiny config and it seems to be limited, some things that you can do in default Tiny just can't be done in Umbraco's Tiny.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Yep i'll update the RFC to be more clear about styles. We plan on adding normal styling directly to the toolbar like headings, quotes, etc... so you don't need to worry about those common things but yes it was also the intention to investigate how we can better deal with styles and and elements and to be able to configure block vs inline, etc... There's probably quite a few ways to do this and the intentions is definitely that each RTE is individually configured - the RFC mentions this but we'll make it more clear.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

HI all, the RFC has been updated. There's a couple of sections in the detailed design that were missing that for some reason i thought were there (my bad sorry!). The updates take into account: Configuration per data type, Improve the HTML for images and Improve common and custom styles/formatting.

@sniffdk

This comment has been minimized.

Copy link

commented Aug 9, 2019

Thanks for the update, definitely important things to include 👍

@sniffdk

This comment has been minimized.

Copy link

commented Aug 9, 2019

Just realized Tiny has a resize function! How awesome would it be if Umbraco supported that! 🙏
image

@Swimburger

This comment has been minimized.

Copy link

commented Aug 9, 2019

@sniffdk A resize option would be great. Alternatively, a full screen button in the TinyMCE toolbar would be great too. It could popup and use more of the screen, or all of the screen.
When writing a lot of content in TinyMCE, I often can't reach the toolbar and have to scroll up.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@Swimburger + @sniffdk the full screen open is there in 8.1

@BatJan

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

One more thing... If one wants to extend the RTE with a custom plugin for the toolbar could it then be possible to more easily do it by placing a file in the "App_plugins" folder?

In this example https://our.umbraco.com/forum/umbraco-7/using-umbraco-7/71490-custom-button-at-tinymce#comment-238680 the plugin is placed in this path \Umbraco\lib\tinymce\ - But obviously it would be really nice to avoid this since you want your custom stuff to stay outside Umbraco core.

I know this can be done using something like this

angular.module('umbraco')
.run(function(assetsService) {
  if (typeof tinymce === "undefined") { // Don't reload tinymce if already loaded      assetsService.loadJs("lib/tinymce/tinymce.min.js")
        .then(function() {
          // Plugin code goes here
        });
  }
});

In the App_Plugins folder where appropriate but it's a bit of a hack really :)

@hemraker hemraker merged commit fceedd4 into umbraco:master Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.