-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Make the TinyMCE help plugin available as an option #59
Conversation
@rber474 thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment:
To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
@jenkins-plone-org please run jobs |
@petschki, the review is needed or can this pull request be merged? Sometimes I am not sure about the workflow. |
Sorry but as mentioned in the original PR here #42 this needs an upgrade step for existing installations, otherwise the vocabulary stored in the registry doesn't know the new token. All in all @rber474 I wonder why you've once again created a new PRs instead of reviewing the existent one which was created before by @ewohnlich ? |
I tried, but the tests did not pass due to the verification of the signed agreement. Initially, I thought it was failing because of @ewohnlich's commits. However, it turned out that the failure was due to my email not being set up correctly on the company laptop. Therefore, I recreated PR without any other goal or attempt to appropriate its work. If it could be, forget this PR and get back to @ewohnlich's Sorry again!! 😞 Regarding the upgrade-step, thought that adding a new value to a source won't need it. At least, no exception was launched when I test in my local. It is not adding a new field, it just a new value that never has been stored. Anyway, I can add it to original PR. Sorry @petschki |
No worries. Steps to reproduce the issue without upgrade step:
now you get a traceback:
This is due to the registry's recordproxy which stores the original vocabulary to the database ... maybe the recordproxy should be more intelligent but unfortunately it isn't ... |
True. Which repository would be the right one to place this upgrade_step? Should be an upgradeStep included in CMFPlone, shouldn't it?
|
Upgrades regarding |
btw. does this help plugin work for you ? in a vanilla classic plone I get a JS error: |
It worked, a help page is shown with the info for the hotkeys. |
Please, revert the PR. I am gonna make it work and check everything twice. |
@petschki I am working in the mockup help plugin for tinymce, to fix URL fail. Currently working in Navigation translations and cleaning up some unused code: |
An upgrade step was still needed, but @1letter meanwhile added this in plone/plone.app.upgrade#324 and plone/plone.app.upgrade#326 |
Just checking if I understand the current status correctly. After I enable the help plugin, I see a Help menu in tinymce: But clicking it does nothing. So the help module only really does something once plone/mockup#1376 is working and merged. |
I'm wondering why the mockup PR is needed because the |
NOTE: If Tiny ships with the plugin and it is activated in the controlpanel, the pattern tries to look it up with an import here: https://github.com/plone/mockup/blob/master/src/pat/tinymce/tinymce--implementation.js#L162 ... to get the log message in case something went wrong, you could add |
FYI I started a page in Plone 6 docs to upgrade to Plone 6.1. plone/documentation#1655 |
Fix issue #41