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

Moving to BS5 with a bit of 3rd party updating #531

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

trollfot
Copy link

This is a PR encompassing a few BS5 update for fields (classes) and a few upgrade of versions (tinyMCE, datepicker).
The PR is presented early to allow comments and change in direction.

@trollfot
Copy link
Author

Addressing : #499

@trollfot
Copy link
Author

Addressing: #452

@trollfot
Copy link
Author

Addressing : #431

@trollfot
Copy link
Author

Addressing: #448

@trollfot
Copy link
Author

I'm currently trying to touch a bit of everything in a "do it first then fix it" kind of way. I'll take care of the tests once we're happy with the fixes. This PR is by no means complete, it allows us to possibly communicate of the different issues. Sorry for the noise, if it produces any.

@stevepiercy stevepiercy marked this pull request as draft November 22, 2023 21:57
@stevepiercy
Copy link
Member

Ah, OK, in that case, I converted this PR to Draft status, which implies, "Please don't review yet, I'm working on it." When ready, you can either @ me or request a review from me and others, and change the status to ready for review.

@trollfot
Copy link
Author

The upload widget is flat out not working, because of the javascript behind it. This needs some heavier work.
In the meantime, checked inputs/password were altered to use an input group. this is a try out, and probably a matter of taste. The previous rendering was very unflattering, so...

@trollfot
Copy link
Author

Starting working on the functional tests. They need a bit of love since the introduction of the html5 "required" changes a lot of the validation tests.

…om js and adapting to the current bootstrap.
@trollfot
Copy link
Author

I've added a "disable_html5_validation" function that put a "novalidate" on the form. It helped me reinstate the previous tests and add new ones, taking care of the html5 'required' and 'validationMessage' control.

I fixed the file upload by removing the old boostrap3 "custom" upload javascript.

There is one thing still problematic : tinyMCE is no longer pre-loadable, it seems. I read wikis and forums, and i didn't find a way to make it work.

Oh, and another thing : html5 validation messages are translatable => should they be translated when we switch the language ? It should be based on the browser's language, but it's customable.

@delijati
Copy link
Contributor

@trollfot maybe it helps i had the same issue in the function test and solved it my manually scrolling a bit https://github.com/Pylons/deformdemo/pull/128/files#diff-e1608f44e2798812f15d2c4a50224a8eabd5e8604a8bffbf981523345ae6e130R3369

@stevepiercy
Copy link
Member

Oh, and another thing : html5 validation messages are translatable => should they be translated when we switch the language ? It should be based on the browser's language, but it's customable.

Indeed, they should be translatable.

BTW, I really appreciate all the work y'all are putting into this PR. I've been swamped with $WORK, and have a couple more weeks until I can adequately address progress and carve out time to make that alpha release I've been promising.

@trollfot
Copy link
Author

@delijati What issue are you talking about ? The code you referenced is already there, since it's based on the min branch where your code was already merged. Am I missing something ?

@delijati
Copy link
Contributor

delijati commented Dec 15, 2023

@delijati What issue are you talking about ? The code you referenced is already there, since it's based on the min branch where your code was already merged. Am I missing something ?

this branch here aka this test is failing https://github.com/Pylons/deform/actions/runs/7199539548/job/19611426675?pr=531#step:7:607

the link i posted was just an example on how the test could be fixed

@trollfot
Copy link
Author

@delijati The failing test is "FAILED deformdemo/test.py::DelayedRichTextWidgetTests::test_submit_filled"n due to the update of TinyMCE. I guess i'm totally missing the point

@delijati
Copy link
Contributor

@delijati The failing test is "FAILED deformdemo/test.py::DelayedRichTextWidgetTests::test_submit_filled"n due to the update of TinyMCE. I guess i'm totally missing the point

No problem, the tinymce test is failing because tinymce is not in "view":

E       selenium.common.exceptions.ElementNotInteractableException: Message: Element <body id="tinymce" class="mce-content-body form-control"> could not be scrolled into view
E       Stacktrace:
E       RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8
E       WebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:189:5
E       ElementNotInteractableError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:349:5
E       webdriverClickElement@chrome://remote/content/marionette/interaction.sys.mjs:166:11
E       interaction.clickElement@chrome://remote/content/marionette/interaction.sys.mjs:135:11
E       clickElement@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:202:29
E       receiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:84:31

What i'm suggesting is a fix i did on some other tests to scroll manually into view

ActionChains(browser).scroll_by_amount(0, 200).perform()

@delijati
Copy link
Contributor

@trollfot do you have some time to look into the remaining issues?

@trollfot
Copy link
Author

@delijati I tried the method you gave me but I couldn't get the test to pass. I think it's the last problem

@delijati
Copy link
Contributor

@delijati I tried the method you gave me but I couldn't get the test to pass. I think it's the last problem

Ok i will also try to find some time to look into it :)

@trollfot
Copy link
Author

@delijati I have time for this, we really want it merged and moving forward

@trollfot trollfot marked this pull request as ready for review January 31, 2024 14:22
@delijati
Copy link
Contributor

Hi i got the last functional test [1] to work. The problem is that the iframe inside of tinymce has a height of 0 so it is not displayed and can't be edited , is seams that the new tinymce can't calculated the height if the element is "hidden" aka display=none. My solution [2], [3] is to remove the preload span and just work with the textarea element itself.

This is also not really perfect as the text in the textarea will be shown with its html tags. Also do we really need this feature?

[1] error

=========================== short test summary info ============================
FAILED deformdemo/test.py::DelayedRichTextWidgetTests::test_submit_filled - s...
============ 1 failed, 284 passed, 7 warnings in 328.68s (0:05:28) =============

[2] deformdemo

diff --git a/deformdemo/test.py b/deformdemo/test.py
index ab4bfa2..c6eaca6 100644
--- a/deformdemo/test.py
+++ b/deformdemo/test.py
@@ -3398,7 +3398,7 @@ class DelayedRichTextWidgetTests(Base, unittest.TestCase):
     url = test_url("/delayed_richtext/")
 
     def test_submit_filled(self):
-        findcss(".tinymce-preload").click()
+        findcss(".tinymce").click()
         time.sleep(0.5)
         browser.switch_to.frame(browser.find_element(By.TAG_NAME, "iframe"))
         ActionChains(browser).scroll_by_amount(0, 200).perform()

[3] deform

diff --git a/deform/templates/richtext.pt b/deform/templates/richtext.pt
index 63fca94..dd17a54 100644
--- a/deform/templates/richtext.pt
+++ b/deform/templates/richtext.pt
@@ -7,13 +7,6 @@
     i18n:domain="deform"
     tal:omit-tag="">
 
-    <style type="text/css">
-      .deform .tinymce-preload{
-          border: 1px solid #CCC;
-          height: 240px;
-          display: block;
-      }
-    </style>
     <textarea id="${oid}" name="${name}"
               tal:content="cstruct"
               class="tinymce form-control ${field.error and error_class or ''}" />
@@ -22,9 +15,6 @@
     that might result in non-HTML-escaped interpolation of
     user input.
   -->
-  <span id="${oid}-preload" class="tinymce-preload"
-        tal:condition="delayed_load and not field.error"
-        tal:content="structure cstruct" />
   <script type="text/javascript">
     (function($){
       deform.addCallback('${oid}', function(oid) {
@@ -35,16 +25,12 @@
           selector: '#' + oid
         };
         var jqoid = $('#' + oid);
-        var jqoid_preload = $('#' + oid + '-preload');
-        if (jqoid_preload.length == 0) {
-          tinyMCE.init(options);
-        } else {
-          jqoid.hide();
-          jqoid_preload.one('click', function(){
-            jqoid.show();
-            jqoid_preload.remove();
+        if ('${delayed_load and not field.error}' == 'True') {
+          jqoid.one('click', function(){
             tinyMCE.init(options);
           });
+        } else {
+          tinyMCE.init(options);
         }
       });
       $().bind('form.pre.serialize', function(event, $form, options) {

@trollfot
Copy link
Author

Tests are green

@trollfot
Copy link
Author

@stevepiercy Can we review this and get it moving ?

@delijati
Copy link
Contributor

delijati commented Feb 27, 2024

I have setup a deform demo server so it is easier to cross check :) When we can merge i will stop the instance.

NEW:

http://5.75.157.57/

OLD:

https://deformdemo.pylonsproject.org/

@delijati
Copy link
Contributor

delijati commented Mar 4, 2024

@stevepiercy sorry to bother you but can we get a ETA when this can be reviewed and merged?

@trollfot
Copy link
Author

Could we get the ball rolling ? It's been a while :)

@stevepiercy
Copy link
Member

I no longer use deform in any of my client projects. I've pinged the other maintainers here @miohtama @mcdonc @ericof to see if they have any interest in taking it on.

We could also take on new maintainers. Would you be interested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants