-
Notifications
You must be signed in to change notification settings - Fork 11
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
DEP Upgrade frontend build stack #82
Conversation
"merge": "^2.1.1", | ||
"mime": "^1.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mime isn't used
"babel-jest": "^20.0.3", | ||
"bootstrap": "^4.3.1", | ||
"jest-cli": "^19.0.2" |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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)
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. We'll need to either:
We should also add behat tests for all of the js functionality in this module. |
343c445
to
163992b
Compare
_config.php
Outdated
$cwpEditor = HtmlEditorConfig::get('cwp'); | ||
$cwpEditor = TinyMCEConfig::get('cwp'); |
There was a problem hiding this comment.
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.
.popover { | ||
max-width: 380px; | ||
width: 380px; | ||
} |
There was a problem hiding this comment.
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.
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:
|
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) There is an existing 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 Note that is would not be feasible to set this to a yaml config since it requires calling methods on the I will call all of this out in the changelog. |
163992b
to
d83c77e
Compare
d83c77e
to
59fa08f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally
See silverstripe/silverstripe-admin#1389 for a wider discussion on most of the changes
Parent issue