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

DEP Upgrade frontend build stack #82

Merged

Conversation

GuySartorelli
Copy link
Member

See silverstripe/silverstripe-admin#1389 for a wider discussion on most of the changes

Parent issue

@GuySartorelli GuySartorelli marked this pull request as draft January 18, 2023 01:50
"merge": "^2.1.1",
"mime": "^1.4.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

mime isn't used

Comment on lines -33 to -35
"babel-jest": "^20.0.3",
"bootstrap": "^4.3.1",
"jest-cli": "^19.0.2"
Copy link
Member Author

Choose a reason for hiding this comment

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

Neither jest nor bootstrap is used

@@ -111,7 +111,7 @@ class ColorPickerField extends Component {
}
}

ColorPickerField.proptypes = {
ColorPickerField.propTypes = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes error Typo in static class property declaration react/no-typos (see the docs on this rule)

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 18, 2023

This module includes a tinymce4 fontawesome4 plugin in a thirdparty directory. This is wildly outdated, and the source code on github isn't much better.
I've found a few tinymce5 plugins that provide similar functionality but nothing for tinymce 6.

We'll need to either:

  1. Fork an existing tinymce fontawesome plugin and update it to be compatible with tinymce 6
    • not the one in thirdparty 'cause that's way too old now - but there are some that are compatible with tinymce 5 which might be easy-ish to upgrade
  2. Make our own tinymce fontawesome plugin and maintain it
  3. Ditch the fontawesome tinymce plugin for this module

We should also add behat tests for all of the js functionality in this module.

_config.php Outdated
$cwpEditor = HtmlEditorConfig::get('cwp');
$cwpEditor = TinyMCEConfig::get('cwp');
Copy link
Member Author

Choose a reason for hiding this comment

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

HtmlEditorConfig doesn't actually have some of the methods that are being called - this is explicitly for tinymce configuration.

Comment on lines +47 to +50
.popover {
max-width: 380px;
width: 380px;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like whatever controls the underlying popover has changed its markup - this is now a div below the .color-picker-field-popover div. If we keep it as it was, there is 380px of whitespace to the right of the form any time you open one of the dropdowns.

@maxime-rainville
Copy link

This module includes a tinymce4 fontawesome4 plugin in a thirdparty directory. This is wildly outdated, and the source code on github isn't much better.

My assumption here is that whoever includes FA icons in a WYSIWYG probably use them effectively as emojis. When FA first came on the scene, Emojis were not as widely supported as they are today. That's probably why no one bothered porting those those FA plugins to work with TinyMCE6.

TinyMCE6 has a free Emoticon plugin.

My suggested approach at this stage is:

  • Remove the FA plugin on CMS6.
  • Maybe add some extra logic just to make sure that existing FA icon still show in the HTMLEditor and in the front end.
  • Maybe enable the Emoticon plugin in the CWP Agency Extension module.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 19, 2023

I will remove the legacy font-awesome plugin.

I will not enable the emoticon plugin, as I see that as being for a fundamentally different use-case (there aren't even any emoticons/emojis in font-awesome - see the icons in the icons list compared with the tinymce emoji - there's not a lot of crossover other than I guess the objects category)
I will call it out as a potential alternative that developers can enable if they think it is suitable for their project.

There is an existing CWP_AGENCY_DISABLE_FONTAWESOME_PLUGIN environment variable which, if set, disables the plugin. I'm going to flip this on its head and replace it with CWP_AGENCY_ENABLE_FONTAWESOME_STYLES.
If the new variable is set, then the extended_valid_elements configuration and font awesome css (from a cdn) will be added to the tinymce config like it used to be. If that is not set, then no font awesome configuration or css will be added at all.

You already had to add css on the front-end separately, so that will not be affected. Projects using this module which wish to keep any existing font-awesome icons in their content will need to set CWP_AGENCY_ENABLE_FONTAWESOME_STYLES to true in their environment variables to see the icons in the CMS WYSIWYG, and to ensure the icons are retained when saving.

Note that is would not be feasible to set this to a yaml config since it requires calling methods on the TinyMCEConfig in _config.php, which happens before some yaml config is set. If there are any projects which cannot set environment variables, they can set the appropriate config and inject the css themselves for their project, which is trivial.

I will call all of this out in the changelog.

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.

Tested locally

@emteknetnz emteknetnz merged commit 8e40f56 into silverstripe:3 Jan 31, 2023
@emteknetnz emteknetnz deleted the pulls/3/frontend-build-stack branch January 31, 2023 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants