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.

Sign up for GitHub

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

Jump to bottom

NEW Migrate TinyMCE code and config to new repo #1

Draft
wants to merge 1 commit into
base: 1.0
Choose a base branch
from
Draft

NEW Migrate TinyMCE code and config to new repo #1

wants to merge 1 commit into from

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli marked this pull request as draft February 12, 2025 21:31
@GuySartorelli GuySartorelli mentioned this pull request Feb 12, 2025
Open
10 tasks
@GuySartorelli GuySartorelli force-pushed the pulls/1.0/migrate-here branch 8 times, most recently from ad8627d to 2246e98 Compare February 25, 2025 00:29
@GuySartorelli GuySartorelli force-pushed the pulls/1.0/migrate-here branch 9 times, most recently from 004527d to 83b9f11 Compare March 4, 2025 05:40
@GuySartorelli GuySartorelli force-pushed the pulls/1.0/migrate-here branch 10 times, most recently from d62075b to 1e05bbf Compare March 10, 2025 05:14
GuySartorelli
GuySartorelli commented Mar 10, 2025
View reviewed changes
_config.php Outdated
$editorConfig->removeButtons('anchor');
$editorConfig->insertButtonsAfter('sslink', 'anchor');
}
$editorConfig->setOption('skin_url', $module->getResource('client/dist/tinymce/skins/ui/silverstripe')->getURL());
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
$editorConfig->setOption('skin_url', $module->getResource('client/dist/tinymce/skins/ui/silverstripe')->getURL());

Oops, this was a debug line. Will remove this after your review as I assume there will be other changes to make at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

GuySartorelli
GuySartorelli commented Mar 10, 2025
View reviewed changes
GuySartorelli
GuySartorelli commented Mar 10, 2025
View reviewed changes
GuySartorelli
GuySartorelli commented Mar 10, 2025
View reviewed changes
emteknetnz
emteknetnz requested changes Mar 11, 2025
View reviewed changes
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

This was an initial peer review which I wanted to submit now so I didn't loose any feedback. I may submit some more feedback on this PR in the near future.

docs/en/01_Configuration.md Outdated
Comment on lines 38 to 40
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->insertButtonsAfter('charmap', 'ssmacron');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->insertButtonsAfter('charmap', 'ssmacron');
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->insertButtonsAfter('charmap', 'ssmacron');

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a standard for this - some docs always have the gap, some omit it when there's very little code.
I'll add the gap to avoid unnecessary ping pong.

docs/en/01_Configuration.md Outdated
Comment on lines 46 to 49
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->removeButtons('tablecontrols', 'blockquote', 'hr');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->removeButtons('tablecontrols', 'blockquote', 'hr');
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->removeButtons('tablecontrols', 'blockquote', 'hr');

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

docs/en/01_Configuration.md Outdated
Comment on lines 62 to 66
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->enablePlugins(['myplugin' => 'app/javascript/myplugin/editor_plugin.js']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->enablePlugins(['myplugin' => 'app/javascript/myplugin/editor_plugin.js']);
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->enablePlugins(['myplugin' => 'app/javascript/myplugin/editor_plugin.js']);

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

docs/en/01_Configuration.md Outdated
Comment on lines 106 to 111
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('my-config')->setOption('resize', false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('my-config')->setOption('resize', false);
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('my-config')->setOption('resize', false);

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

docs/en/01_Configuration.md Outdated
Comment on lines 162 to 168
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->disablePlugins('ssembed');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->disablePlugins('ssembed');
use SilverStripe\TinyMCE\TinyMCEConfig;
TinyMCEConfig::get('cms')->disablePlugins('ssembed');

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

tests/behat/features/apply-formatting.feature Outdated
@@ -0,0 +1,29 @@
@retry @job1
Copy link
Member

Choose a reason for hiding this comment

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

Remove all of the @job1 annotations from all the behat files

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary, and confused me thinking there were two jobs, when there is only one

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

webpack.config.js Outdated
'TinyMCE_sslink': `${PATHS.SRC}/plugins/TinyMCE_sslink.js`,
'TinyMCE_sslink-external': `${PATHS.SRC}/plugins/TinyMCE_sslink-external.js`,
'TinyMCE_sslink-email': `${PATHS.SRC}/plugins/TinyMCE_sslink-email.js`,
// asset plugins
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// asset plugins
// asset-admin plugins

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

webpack.config.js Outdated
// sass to css
new CssWebpackConfig('css', PATHS)
.setEntry({
// bundle: `${PATHS.SRC}/styles/bundle.scss`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// bundle: `${PATHS.SRC}/styles/bundle.scss`,

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

.github/workflows/ci.yml
@@ -0,0 +1,10 @@
name: CI
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we're missing things like auto merge-ups. Did you run module-standardiser? If not could you add as a "do later" AC on the parent issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Running standardiser before there's any code or config files will at best give incomplete results. There's a note under "Still needs to be done" in the issue for this.

This was referenced Mar 11, 2025
Merged
Merged
emteknetnz
emteknetnz requested changes Mar 11, 2025
View reviewed changes
_config/config.yml
SilverStripe\Forms\HTMLEditor\HTMLEditorConfig:
default_config_definitions:
cms:
configClass: 'SilverStripe\TinyMCE\TinyMCEConfig'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
configClass: 'SilverStripe\TinyMCE\TinyMCEConfig'
configClass: SilverStripe\TinyMCE\TinyMCEConfig

Don't need quotes for an FQCN

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussion about that can be in silverstripe/silverstripe-framework#11617 (comment)

@GuySartorelli GuySartorelli force-pushed the pulls/1.0/migrate-here branch 2 times, most recently from aff183e to 191068a Compare March 11, 2025 22:47
@GuySartorelli GuySartorelli force-pushed the pulls/1.0/migrate-here branch from 191068a to 8684a8e Compare March 12, 2025 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emteknetnz emteknetnz emteknetnz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GuySartorelli @emteknetnz