|
Description
Katrin Fischer
2019-04-08 18:14:07 UTC
Lucas proposed a good idea on bug 24764: Use CodeMirror! Changing the bug title to advertise the idea a bit more. With more system preferences like OPACHeader(bz22880) and OPACMainUserBlock (bz23794) moving into the News feature there needs to be a robust way of editing HTML, I think CodeMirror is appropriate. Personally, I'd prefer to use CodeMirror. However, my librarians would rather use a WYSIWYG editor I think, so I think being able to do an either/or would be great. Created attachment 104624 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE This fails on step 3 - neither WYSIWYG or CodeMirror loads after applying the patch. Instead, it's a plain textbox rather than the numbered lines you see with CodeMirror. If you're editing an existing news item, the plain textbox contains HTML. When you check NewsToolEditor, it has defaulted to CodeMirror rather than staying as WYSIWYG after the patch has applied. Created attachment 104948 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE Sally, Thank you for testing. I made some corrections that should prevent what happened to you in step 3. I also updated the test plan to remind testers to run updatedatabase after applying the patch. I have reset the bug status to 'Needs Signoff'. Created attachment 104951 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and make sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE (In reply to Lucas Gass from comment #7) > Sally, > > Thank you for testing. I made some corrections that should prevent what > happened to you in step 3. I also updated the test plan to remind testers to > run updatedatabase after applying the patch. > > I have reset the bug status to 'Needs Signoff'. Great, thanks Lucas - I'll give it another go. I think I'm going to have to admit defeat - I can't see how to update the database using a sandbox, and from your comments, I think that's the cause of the errors I'm seeing. I'll leave it for someone else to test - it would be a useful feature. :) Created attachment 105129 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE Created attachment 105158 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE Created attachment 107267 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 107268 [details] [review] Bug 22660: (follow-up) Improve asset handling, add linting This patch makes some changes to the way assets are included based on the value of the system preferece in order to minimize unnecessary JavaScript includes. The patch also adds HTML syntax-highlighting and linting like we have in the system preferences editor. Removed are two JS files which were not required. To test, apply the patch and set the NewsToolEditor preference to "CodeMirror." - Edit a news item and confirm that the contents of the editor show HTML syntax highlighting. - Test HTML linting by adding some malformed HTML (missing closing tag, for instance). The error should be highlighted. - Set the NewsToolEditor to "TinyMCE" and confirm that WYSIWYG editing still works correctly. Created attachment 107269 [details] [review] Bug 22660: (follow-up) Improve asset handling, add linting This patch makes some changes to the way assets are included based on the value of the system preferece in order to minimize unnecessary JavaScript includes. The patch also adds HTML syntax-highlighting and linting like we have in the system preferences editor. Removed are two JS files which were not required. To test, apply the patch and set the NewsToolEditor preference to "CodeMirror." - Edit a news item and confirm that the contents of the editor show HTML syntax highlighting. - Test HTML linting by adding some malformed HTML (missing closing tag, for instance). The error should be highlighted. - Set the NewsToolEditor to "TinyMCE" and confirm that WYSIWYG editing still works correctly. https://bugs.koha-community.org/show_bug.cgi?id=22600 Created attachment 107270 [details] [review] Bug 22660: (follow-up) Re-word system preference description This patch rewords the system preference description to focus on the generic description of the editor rather than the name of the plugin: tinymce: "a WYSIWYG editor (TinyMCE)" codemirror: "a text editor (CodeMirror)" Created attachment 107361 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Created attachment 107362 [details] [review] Bug 22660: (follow-up) Improve asset handling, add linting This patch makes some changes to the way assets are included based on the value of the system preferece in order to minimize unnecessary JavaScript includes. The patch also adds HTML syntax-highlighting and linting like we have in the system preferences editor. Removed are two JS files which were not required. To test, apply the patch and set the NewsToolEditor preference to "CodeMirror." - Edit a news item and confirm that the contents of the editor show HTML syntax highlighting. - Test HTML linting by adding some malformed HTML (missing closing tag, for instance). The error should be highlighted. - Set the NewsToolEditor to "TinyMCE" and confirm that WYSIWYG editing still works correctly. https://bugs.koha-community.org/show_bug.cgi?id=22600 Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Created attachment 107363 [details] [review] Bug 22660: (follow-up) Re-word system preference description This patch rewords the system preference description to focus on the generic description of the editor rather than the name of the plugin: tinymce: "a WYSIWYG editor (TinyMCE)" codemirror: "a text editor (CodeMirror)" Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Worked great! I switched back and forth a few times making different changes each time and everything worked well with both editors. Created attachment 107554 [details] [review] Bug 22660: Add system pref to allow switching of editors in News Tool This patch adds a system preference called 'NewsToolEditor' which allows for the choice between TinyMCE and CodeMirror in the News Feature. TEST PLAN: 1. Apply patch and run updatedatabase 2. Go to Tools > News and make a new entry or edit an existing News item. 3. When you initially load you should gte the TinyMCE editor (WYSIWYG) 4. Search for the NewsToolEditor system preference and switch to CodeMirror. 5. Reload your News item and now you should see the CodeMirror editor. 6. Try saving content with both and maki sure it works. 7. Try adding some different HTML tags in CodeMirror mode, saving, and making sure it looks right when you switch back to TinyMCE Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 107555 [details] [review] Bug 22660: (follow-up) Improve asset handling, add linting This patch makes some changes to the way assets are included based on the value of the system preferece in order to minimize unnecessary JavaScript includes. The patch also adds HTML syntax-highlighting and linting like we have in the system preferences editor. Removed are two JS files which were not required. To test, apply the patch and set the NewsToolEditor preference to "CodeMirror." - Edit a news item and confirm that the contents of the editor show HTML syntax highlighting. - Test HTML linting by adding some malformed HTML (missing closing tag, for instance). The error should be highlighted. - Set the NewsToolEditor to "TinyMCE" and confirm that WYSIWYG editing still works correctly. https://bugs.koha-community.org/show_bug.cgi?id=22600 Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 107556 [details] [review] Bug 22660: (follow-up) Re-word system preference description This patch rewords the system preference description to focus on the generic description of the editor rather than the name of the plugin: tinymce: "a WYSIWYG editor (TinyMCE)" codemirror: "a text editor (CodeMirror)" Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 107557 [details] [review] Bug 22660: (QA follow-up) Fix capitalization in system preference Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I think the CodeMirror option is much needed as we move more OPAC prefs, so really happy to see these patches. I think a nice improvement could be to have a "both" option - making both available and allow to choose, depending on what you are editing (former pref or news entry) While I and my coworkers will probably prefer HTML for editing the OPAC related bits, the libraries we support will more likely prefer the WYSIWYG for adding/editing news. Maybe by having 2 different Edit buttons/options or a split button, defaulting to the editor from the pref? Created attachment 107592 [details] [review] Bug 22660: (QA follow-up) Fix capitalization in system preference Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (fixing patch's author) Pushed to master for 20.11, thanks to everybody involved! Oops, thx Jonathan :) If you work with the HTML system prefs a lot, which are moving into the News tool, this enhancement could be pretty important. Choosing to backport to 20.05.x for 20.05.03 enhancement changing too much behaviour, not backported to 19.11.x |