#e2e

My walls of text made it tough to navigate

#33 ends with me saying we need a test here that confirms the MR successfully filters _none out of required enums - sounds like you agree

So there is in fact a working fix for required enums that needs e2e tests. Probably all that is needed is confirming there's no _none option in a required select.

This suggests test coverage is missing?

But then #34 refers to item 6 in #12, suggesting that the "locking" test coverage should be removed?

(I agree that the "locking" bits should be left for a later issue, because they require design, whereas this is a purely functional fix.)

I'm sorry, but it's not clear to me yet what the next step is that you recommend 🙈

In both 0.x and the MR, a required field with no other schema restraints will not crash when emptied - all the crashes we'd been running into are because there was a minLength requirement not being met.

In addition to it not crashing #31 demonstrates that a required field can be emptied and the preview will update with that empty value. Perhaps we should be doing something in real time about this, or perhaps it's OK to just prevent saving, which the already-added required attributes would satisfy.

Item 6 in comment #12 - "locking" the UI already has tests written for it in anticipation of its implementation but this specific functionality seems like it would be best to do in a separate issue as there's no (AFAIK) existing UI-lock functionality and there are probably some design discussions needed to determine how this locked UI would be conveyed to the user - or maybe it's simpler than I'm hunching. If it seems worth adding to this issue, lets get it in the IS

What was implemented for enum + _none is as-discussed and continues to work that way. Selecting _none in a select does removes field from the model. The recent refactoring still effectively does this, but instead of looking for _none to determine model-exclusion, it will respond to any value not defined by the enum This is the e2e that confirms selecting _none works.

That approach does not work for a required enum prop, because it is fully removing something required from the model . I think the best solution here is to disallow _none entirely on required select fields - if it's required then _none isn't a valid option and shouldn't be presented to them anyway.

That's what @traviscarden worked on. He did that only for required props, which is what this issue was prescribing.

There is logic in the current MR that adds the required attribute to required fields, that's good! In the case of enum/select + required I think it's best to prevent invalid options from being available so we don't have to rely on FE catching it or have users wondering why a select offers an invalid option.

so it may be best to make that option unavailable before it reaches the front end.

That's what @traviscarden worked on. He did that only for required props, which is what this issue was prescribing.

I previously investigated the _none case for enum: […] prop shapes (write-up at #3471511-13: `enum` data shapes: error when choosing "- None -" in `` → ). I'll repeat the conclusion here for clarity:

Conclusion

Select.tsx, introduced in 🐛 Prop select lists don't affect the component Fixed , needs to be updated to be aware of this special value. If this special value is selected, then:

1. the UI should interpret this as "no input specified by user"
2. the UI should then delete this prop from the model, rather than setting "_none" as the value — because that's the whole point of ✨ Contextual form values need to be integrated with Redux Active : to allow real-time updates of the preview based on information on the client side only
3. that will then prevent this invalid model to be sent from the client to the server:

@bnjmnm: over at #3471511-15: `enum` data shapes: error when choosing "- None -" in `

` → you said you had solved that already. But … then earlier yesterday, we merged 🐛 XBEndpointRenderer & processResponseAssets() do not support `ajaxPageState` ⇒ duplicate CSS/JS loading Needs review , which changed the _none handling 😅 I suspect that's where there probably is a mismatch? 🤓 What is your updated assessment/recommendation compared to your comment #15 at #3471511? 🙏

The issue as reported isn't a problem.
The My Hero header prop is required and has this property set: minLength: 2
If I remove the minLength configuration, it handles the empty required field without a crash

I'm sure required fields could offer a better experience, but there's no issue with crashing - it's an entirely different set of prop requirements causing the crash.

______________________________

The IS also mentions enum. Required enum fields will crash if _none is chosen so it may be best to make that option unavailable before it reaches the front end.

__________________

Based on the new info I'm not sure how much existing work can be re-used. I'll leave that to the folks who actively worked on the branch. The issue summary should be updated to this more accurate info + the intended approach to fixing it.

but please add one example at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3..., that'll help the next person reading that file

Example added

wim leers → closed merge request !279

wim leers → closed merge request !306

RTBC — but please add one example at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3..., that'll help the next person reading that file 😊

Feeback addressed. Left threads open for anything with an explanation that might require clarification.

The e2e tests (and I assume most/all manual ones) were being performed on the header field in the hero component, which has a minimum length requirement in addition to being required.

A quick check of the logs makes this evident

  Drupal\Core\Render\Component\Exception\InvalidComponentException: [heading] Must be at least 2 characters long in  
                                        Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of                                       
                                        /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php). 

That min length requirement is not important, so it was removed by me (then again by Wim when it was overwritten) and the tests are now testing an input where required is the only thing being checked against.

Reviewed! Looks great! There's two bits that I don't understand what they do in there, and I'm hopeful you'll be able to just remove them 🤞

New MR combining the prior solutions is ready.

While manual testing, there's still a strong chance you'll encounter things that don't fully work as expected due to this entire project still being in the "under construction" phase.

If they appear to be problems introduced by the solution in the MR, lets address them in this issue

If they are issues not *specific* to the symptoms in the issue summary and not *introduced* by the MR - first see if there is an existing issue filed and if not file one and no need to even mention it here. The changes in this MR are very beneficial and the more we can streamline communication the faster this will land.

bnjmnm → changed the visibility of the branch 0.x to hidden.

bnjmnm → changed the visibility of the branch 3472900-fix-empty-js to hidden.

Bumping to because without it, our E2E tests do not cover the full spectrum of typical use.

Plus, it already slowed velocity down once before — even though the root cause was a simple back-end fix.

@bnjmnm and I pair-reviewed this to determine next steps.

He's going to combine the excellent work in both MRs (👏 @boulaffasae especially! 😄).

Ben posted detailed feedback on the test coverage. The test coverage needs work to improve its reliability. Could you address that feedback, @soaratul? 🙏

Hi @kristen, @longwave

The issue is much bigger than just a CSS/JS loading. @longwace MR will help us build a demo for the upcoming event, and in the meantime I will try to continue with the ajax_page_state approche to resolve the whole issue.

1. Media Library duplicate JS/CSS files, which result in overriding definition (Yes! when Media Library load ajax.js, it automatically erase every definition in dialog.ajax.js - therefore openDialog become undefined and nothing happen on ajax)
2. We can filter existing JS, but then LoadJS library does throw an issue when we pass an empty Array.

-- longwave MR resolve these two

3. Wim suggested that we use ajax_page_state, we need it's logic to skip loading an existing library JS/CSS. But this conflict with Radix state.
4. AjaxResponseAttachmentsProcessor return empty Ajax settings after inserting a Media just because all JS/CSS files are already downloaded.
5. Fied media is not yet supported in Experience Builder (uploading an image might result in it not show up in Preview)

and now it seems like we have new issues with Select too (like we did with Textarea before)

Just two people working on it with slightly different approaches - mine solved exactly the issue in the issue summary but there are wider problems with adding JS when widget forms are loaded in, which the other MR is trying to solve.

I think the issue should be split in two to solve the immediate UI issue here and then fix the bigger Ajax issue separately, but I don't think Wim agrees with this.

I didn't mean to change the status. Not sure what happened there.

Okay... so I'm confused... too many comments.

Why do we have both MRs? Should they still both be open?

Let me know if either of these should be be tested again at some point.

Will the front end folks determine the priority. Though the problem with an empty canvas discovered in #3472299-20: Update default config to make a fresh install result in an XB UI with an empty canvas → was actually a backend issue.

I implemented #4.3 - the app no longer gets stuck in a loop if you empty a required field - but have no idea where to start on the rest of #4 so leaving this again for now.

FWIW prop-types.cy is now failing in both MRs due to 🐛 Emptying a required value through the UI crashes the app Active when testing the enum and setting the value to "None".

This is the small piece of code that I added to web/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php

    // Issue #3472900 by kristen pol: XB props form hanging for SDDS components
    $found = false;
    foreach ($commands as $entry) {
      if (isset($entry["command"]) && $entry["command"] === "settings") {
        $found = true; break;
      }
    }
    if (!$found) {
      array_unshift($commands, [
        'command' => 'settings',
        'settings' => $assets->getSettings(),
        'merge' => true,
      ]);
    }

Hi everyone, I recorded a demo (Four columns with Cards)

- https://youtu.be/IVyarGnVZOQ

We still have a small issue with Media widget (already addressed in #3473155 https://www.drupal.org/project/experience_builder/issues/3473155 📌 Redux Sync all single-value types in the SDC test all props form Fixed - but they only added a warning about it in console warns "The field media does not yet support updating the preview on change. It will soon.")

I think I have a fix for that, but for now I went with a small trick to skip it (Uploading a new image in the media - Clicking outside the Card - re-click again on the Card and change the title if you want)

As I concluded at the end of #25, I think the only correct way forward is using ajaxPageState, which @boulaffasae's MR uses, but @longwave's MR does not 😅

So while @longwave's MR might be sufficient to unblock @lauriii's upcoming DrupalCon Barcelona demo recording (yay!), I do think that it makes more sense to continue with @boulaffasae's MR? 🙈

Reviewed @boulaffasae's MR.

@traviscarden won't be working for another several hours, while @soaratul is in the middle of his day! Plus, he wrote the E2E test, so is better suited to update it 🤞

The back-end changes LGTM — now we just need to get the E2E test to pass again 😇

I pushed some code that seems to work locally. I can't run Cypress tests right now for some reason; we'll see what happens on CI. The code probably isn't very elegant, but red => green => refactor, you know. 🙂

Update: I'm on track with a solution, but there's an ugly merge conflict with 0.x in xb-general.cy.js that I'm trying to rebase away.

It took me a few tries, but I made it through all components except the last one in one go... I had to side step some issues (e.g. dragging certain things), but this is SO DEMOABLE if you know what *not* to do :D

https://youtu.be/SAir2OxwMS4

Thanks @longwave (we crossposted so just seeing yours)

Progress!!!

I used a fresh environment and 3472900-fix-empty-js branch.

Everything *appears* to be working except for uploading new media. But I can choose existing media and it works!

I tested with Case Study and Card.

I can create a demo with this by upload images ahead of time and choosing the images.

This is a huge step forward! 🎉 🙏 😍

@kristen pol

1. Install Experience Builder, Media, Media Library
2. On /xb/node/1 add a second Image component to the page
3. Click on one of the image components, the form will work
4. Click on the other image component, the form just spins

On the 3472900-fix-empty-js branch this works for me, both forms work as expected. Remember to npm run build and ddev drush cr after switching branches.

I'll try the steps in #35 first.

#33 has:

MR!306 solves the issue for me

I tried both branches but still get the spinning props form. I'll try installing a completely new environment.

@longwave Which component were you using? Steps for your manual testing would be great.

I'm playing manually again, after uploading an Image in the 1st Icon Card, I add a 2nd Icon Card to the page and before adding a media, I go to web/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php:

I comment the code

//    ->setAlreadyLoadedLibraries(isset($ajax_page_state['libraries']) ? explode(',', $ajax_page_state['libraries']) : [])

and I add these at the end

    unset($commands[1]);
    unset($commands[2]);
    return $commands;

this help me load the settings in the Ajax response + Removing the JS/CSS (they were already loaded in the 1st component edit)

and things work just fine.

Now I need to figure out how to do this programmatically

Hi @wim leers, I added ajax_page_state as you suggested

        "ajax_page_state[libraries]": drupalSettings.ajaxPageState?.libraries,

I added only libraries (adding theme result in an issue - I don't think we need it)

I added setTimeout (because I found out they use it also for Drupal object + It help trigger the functions inside only after page load = until drupalSettings is defined)

** web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php will decompress the string

and then it's up to XBEndpointRenderer to remove the existing CSS/JS from the Array

I still have an issue, Media Library doesn't open - I think it's related to the #2 issue ('insert' doesn't get triggered in ajax.js)

MR!306 solves the issue for me. What was happening is that the (filtered) set of libraries sent to add_js was empty for the second instance of the element, and loadjs() does not resolve the promise when asked to do nothing. The fix is to filter the list of libraries *before* trying to send it to add_js.

I haven't yet got the Cypress tests working locally so I cherry-picked the new test from the previous branch.

This is really nice, thank you, @omkar-pd! 🎉

I was able to reproduce this with the default components that are loaded in XB. It seems that the problem is that you can only use a single component with an image – after highlighting a component, something about the state is off and means that other components with images cannot be edited anymore.

LGTM — but needs final sign-off from one of the /ui owners 😊

A little update

Textarea + Image fix are no more needed (this was resolved in #3473155 📌 Redux Sync all single-value types in the SDC test all props form Fixed )

I will update the PR

// *** HOW TO RESOLVE THE ISSUE MANUALLY *** //

1 // Add an Icon Card and upload an Icon
2 // Now add an Icon Card (but don't open the Contextual Panel)
Go to web/modules/contrib/experience_builder/src/Render/MainContent/XBEndpointRenderer.php, both response->headers->set('Attach-Css', ... and response->headers->set('Attach-Js', ... should be commented (else XB will load the ajax.js a 2nd time which will result in ajaxCommands being override and we will lose the openDialog function definition, this will break the Media functionnalities for the new Icon Card)
3 // Now youn can safely open the Contextual Panel (but don't click the Add Media just yet)
Go to web/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php, and update the setAlreadyLoadedLibraries replace the args with an empty array and then you can click Add Media and everything will work fine

      ->setAlreadyLoadedLibraries([])

// *** WHAT NOW *** //

I think @wim leers suggestions will resolve the 1st issue, I will work on that. Then we will need to find out how to resolve the 2nd issue

2nd issue explanation: When Ajax load the necessary JS/CSS for the Media Library widget, it try to skip loaded libraries but then If no JS get loaded Ajax skip also the drupalSettings part :/ (Yes he do - ajax return only $response->getCommands() which concatenate js_assets_footer, js_assets_header, and css_assets what about $attachments['drupalSettings'] = $assets->getSettings())

Note: this will likely also solve 🐛 Make Media Library widget work when JS aggregation is enabled Active .

wim leers → credited bhuvaneshwar → .

Reported again: 🐛 Selecting "none" option for one of Heading SDC's s crashes the app Closed: duplicate . Crediting @bhuvaneshwar, the reporter.

#18: 👍 — then assigning to @traviscarden, who started working on this yesterday (see #13)

👍

@wim-leers Yes exactly
- #7 has been done with assumption
- We can now move to the subsequent in #5, along with what I've mentioned in #15

Also: I will be resolving the conflicts while verifying the test cases - and test cases can be verified only after completing all the task mentioned in #15 e.g. stop dragging, selecting, rearranging, deleting etc.. while editing component(empty required field value).

RE #12 it looks like the test is still flakey but it's flakey in a slightly different way now! I think that counts as progress.

I've just fixed conflicts so this branch now has the fixes that were added in 📌 Redux Sync all single-value types in the SDC test all props form Fixed but I'm not able to look into this right now as other issues have priority.

Still happening, sadly: https://git.drupalcode.org/project/experience_builder/-/jobs/2727129

What's the status here, @soaratul? AFAICT you finished the test yesterday, does that mean you think that #7 is now done and we can move on to the subsequent steps outlined in #5?

Also: the MR conflicts with HEAD, so it needs a reroll.

Nice — tests exist now! 👍

If two components depend on the same JS, what should happen ?

1/ only load the dependencie one-time, but this will not trigger Drupal.attachBehaviors :/
2/ load the dependencie twice, this will result in a JS file being included two times and will break the click trigger
3/ load the dependecie once, and execute the Drupal.attachBehaviors (something doesn't seem right - the contextual panel stuck at the 1st component props form)

1. We must manually invoke Drupal.attachBehaviors() — that's fine! :) That's how Drupal's AJAX system works too.
2. 👎
3. This should work fine. That's how Drupal's AJAX system works too.

1+3 are actually already happening — see Drupal.behaviors.jsxAjaxProcess() in ajax.hyperscriptify.js :)

Ah … that won't work as soon as we're dealing with aggregated assets! Because then each component with a different set of asset libraries (e.g. A + B + C) than another component but with a different set (e.g. B + C), will cause a different asset URL to be generated, and hence fail.

Conclusion: both the client-side and the server side MUST be updated to use AjaxPageState as explained in #13.