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

[4.0] Tinymce 5.0 migration #24110

Merged
merged 65 commits into from Apr 18, 2019

Conversation

@bembelimen
Copy link
Contributor

commented Mar 6, 2019

Summary of Changes

First approach of TinyMCE 4.x => 5.x migration.
Not everything is ready yet.

Testing Instructions

  • [IMPORTANT]: Apply #24211
  • Activate tinymce and create a new article
  • Test the tinymce builder option
@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Awesome - To save some time in testing. When you say "not everything is ready" can you highlight what

@brianteeman brianteeman referenced this pull request Mar 6, 2019

Closed

[4.0] TinyMCE 5.0 #23251

@bembelimen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

I just tried to get the new version running, so stuff which has to be done (I guess the list is not complete):

  • the height is broken
  • the parameters needs a complete clean up
  • I would like to activate some features like "inline formatting" (when you select a text the small tooltip with the format options)
@dgrammatiko

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@bembelimen you need to do this as custom element as the rest of the fields

@C-Lodder

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

Nice.

Just an FYI, I think there is path in the plugin XML file allowing that's used for the skins directory. This might need updating too as you've added /ui to the other paths

@bembelimen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Just an FYI, I think there is path in the plugin XML file allowing that's used for the skins directory. This might need updating too as you've added /ui to the other paths

Yes, thanks, I updated it.

The next days I'll go through the parameters and fix this stuff. If someone want to help, feel free to submit a PR against my branch :)

bembelimen added some commits Mar 7, 2019

Offer px instead of pt in font-size dropdown
Format parameter view in tinymce
@zwiastunsw

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Is it possible to add an Accessibility Checker plugin?

@bembelimen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Is it possible to add an Accessibility Checker plugin?

Do you have a link to one?

@zwiastunsw

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@bembelimen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Thx, it seems that it is a paid plugin?

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@zwiastunsw that plugin is neither free of cost or free to use

@zwiastunsw

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Sorry, in fact...

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented on plugins/editors/tinymce/form/setoptions.xml in e1fdcb4 Mar 9, 2019

there is an extra tab ;)

bembelimen added some commits Mar 10, 2019

bembelimen added some commits Apr 15, 2019

@bembelimen bembelimen marked this pull request as ready for review Apr 16, 2019

@bembelimen bembelimen requested a review from laoneo as a code owner Apr 16, 2019

bembelimen added some commits Apr 16, 2019

@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<extension version="3.2" type="plugin" group="editors" method="upgrade">
<name>plg_editors_tinymce</name>
<version>4.8.3</version>
<version>~5.0</version>

This comment has been minimized.

Copy link
@wilsonge

wilsonge Apr 16, 2019

Contributor

This is wrong. Also i thought this was automated from the build tools script to replace this

This comment has been minimized.

Copy link
@bembelimen

bembelimen Apr 16, 2019

Author Contributor

Yes, perhaps we should adjust the build script? Otherwise we have to update the version everytime there is a small bugfix...

This comment has been minimized.

This comment has been minimized.

Copy link
@mbabker

mbabker Apr 16, 2019

Member

It uses the value from the package.json dependencies list. Same with CodeMirror.

This comment has been minimized.

Copy link
@mbabker

mbabker Apr 16, 2019

Member

Otherwise we have to update the version everytime there is a small bugfix...

I'm getting out of the conversation after this one. But a lot of people will suggest a best practice of using specific versions in your manifests (composer.json and package.json for Joomla) when you are building an application and version ranges when building a reusable library or package. Which effectively declares the application only supports a specific version of a dependency, which has its pros and cons. Personally, I don't follow that approach, but I also don't regularly build modular applications.

This comment has been minimized.

Copy link
@bembelimen

bembelimen Apr 16, 2019

Author Contributor

I improved the build script, which had a wrong regexp and needed some version cleanup.

bembelimen added some commits Apr 16, 2019

@@ -220,15 +220,17 @@ const copyFiles = (options) => {

copyArrayFiles('', ['tinymce.js', 'tinymce.min.js', 'changelog.txt', 'license.txt'], 'tinymce', '');

let tinyversion = options.dependencies.tinymce.replace(/[^0-9.]/, '');

This comment has been minimized.

Copy link
@hound

hound bot Apr 16, 2019

'tinyversion' is never reassigned. Use 'const' instead prefer-const

@@ -4,7 +4,7 @@ const FsExtra = require('fs-extra');
const Path = require('path');
const RootPath = require('./utils/rootpath.es6.js')._();

const xmlVersionStr = /(<version>)(\d+.\d+.\d+)(<\/version>)/;
const xmlVersionStr = /(<version>)(.+)(<\/version>)/;

This comment has been minimized.

Copy link
@bembelimen

bembelimen Apr 16, 2019

Author Contributor

Reason: versions are not always 3 numbers, could also be less.

@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<extension version="3.2" type="plugin" group="editors" method="upgrade">
<name>plg_editors_tinymce</name>
<version>4.8.3</version>
<version>5.0</version>

This comment has been minimized.

Copy link
@wilsonge

wilsonge Apr 17, 2019

Contributor

This regex still isn't quite working correctly. package-lock.json is showing you have version 5.0.1 so that should be the version in this field

This comment has been minimized.

Copy link
@bembelimen

bembelimen Apr 17, 2019

Author Contributor

I reverted the behavior to "before" and set the version "hard code" into the package.json, so now the coorect version should be displayed.

I'll probably create a new PR to tackle the wrong regexp, but not here...

jeckodevelopment and others added some commits Apr 17, 2019

@uglyeoin

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

When I use patchtester I get

The file marked for modification does not exist: build/build-modules-js/init.es6.js

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

com_patchtester does not work with anything that changes UI assets in 4.0 (anything in the build folder). If you see the "NPM Resource Changed" label on a pull request then don't waste your time trying.

@uglyeoin

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@mbabker thank you. I assume it's not as simple as uploading the files to the right folder?

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

If you can get a copy of the compiled asset tree with the changes from this PR then you can upload that. But it's not as simple as uploading the build directory to a 4.0 installation.

@zwiastunsw

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Step 1. I'm downloading a copy of the branch from the Github of the author of the PR (for ex. https://github.com/bembelimen/joomla-cms/tree/tinymce-5.0)
Step 2. Next I unpacking in the document folder on a local server
Step 3. I'm running a Composer in this folder (composer install).
Step 4. I'm running a NPM in this folder (npm install)
Step 5. I'm installing Joomla

@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

I have tested this item successfully on b3bfa55

Works for me


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24110.

@wilsonge wilsonge merged commit 895eb40 into joomla:4.0-dev Apr 18, 2019

4 of 5 checks passed

JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed.
Details
Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

[4.0] Administration automation moved this from In progress to Done Apr 18, 2019

@wilsonge wilsonge added this to the Joomla 4.0 milestone Apr 18, 2019

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Thank you @bembelimen

@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Indeed thankyou!

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