- Issue created by @Kristen Pol
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Note that this:
๐ Redux support for ImageWidget: `[image] String value found, but an object is required` Postponed
is what crashes things when trying to use a props form that has an image.
The hanging props form doesn't seem related.
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
I added back one of the asserts (below) and rebuilt and the hanging behavior seems pretty much the same. I'll add an issue for the other assert causing fatal error.
--- a/src/Plugin/Field/FieldFormatter/NaiveComponentTreeFormatter.php +++ b/src/Plugin/Field/FieldFormatter/NaiveComponentTreeFormatter.php @@ -24,6 +24,8 @@ class NaiveComponentTreeFormatter extends FormatterBase { * {@inheritdoc} */ public function viewElements(FieldItemListInterface $items, $langcode) { + // @fixme KP + //assert($items->count() === 1); assert($items->count() === 1); assert($items[0] instanceof ComponentTreeItem); // This field type is single-cardinality: delta 0 is rendered.
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Added an issue for that:
๐ฌ AssertionError: assert($component_config !== NULL) Closed: outdated
Hi kristen,
Looking at the Network logs, when you drag a new "Icon Card" and you upload a new file (JPG), the preview get updated with no issue. But right after I change the title It throw an issue
The website encountered an unexpected error. Try again later.<br><br><em class="placeholder">Drupal\Core\Render\Component\Exception\InvalidComponentException</em>: [image] String value found, but an object is required in <em class="placeholder">Drupal\Core\Theme\Component\ComponentValidator->validateProps()</em> (line <em class="placeholder">203</em> of <em class="placeholder">core/lib/Drupal/Core/Theme/Component/ComponentValidator.php</em>). <pre class="backtrace">Drupal\Core\Template\ComponentsTwigExtension->doValidateProps() (Line: 109)
Payload (after changing title field)
{"name":"Icon Card","image":{"alt":"Alternative text ","width":1280,"height":853,"src":"/sites/default/files/2024-09/banner-4887650_1280_1.jpg"},"title":"User-centric experiences","summary":"Create compelling content on more devices than ever. With tools to build versatile, structured content and integrate with a wide array of digital marketing channels.","randomProp":"random"}
Payload
{"name":"Icon Card","image":"","title":"User","summary":"","randomProp":"random"}
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Thanks. Thatโs probably the issue in comment #2 above. This issue is about the prop form hanging
- Merge request !279Issue #3472900 by kristen pol: XB props form hanging for SDDS components โ (Closed) created by boulaffasae
Hi @kisten,
I was able to solve some of the issues
1. [image] String value found, but an object is required
setFormState was settings image to '' (empty string) because the prior state had an entry
xb_component_props[9acb612e-748c-4f69-b07e-66b95e5d173e][image][media_library_selection]
2. Textarea
Textarea wasn't getting updated, I found out that the onChange (from InputBehaviors) doesn't get triggered. I updated the code (+ added value because textarea have an entry value on the properties level)
This still doesn't resolve the issue with multiple instances of Card Icon. And another interesting issue, image doesn't get updated in the preview -_-
I'm sorry for not moving everything into the same issue, but as you know you need to have the #3472634 fix just to scroll to Icon Card and I don't think we can rebase on 3 differents issues just to have the ability to test and resolve this one.
Hi !
It looks like we have an issue on Redux level, after I click on the 2nd Card Item. Redux stuck at status: "pending"
UPDATE
once I commented the below in createApi the issue is gone
transformResponse: processResponseAssets,
Finally, I was able to trace this (well the issue was there all the time "loadjs")
// JS
when you open a 2nd Icon Card, it try to load the same JS libraries that were loaded before. This throw an error, because we have a function that filter the JS array and only load the new JS - and it send an empty string which break loadjs
// CSS
LoadJS throw the below error (not sure why they couldn't add an explanation -_-)
// throw error if bundle is already defined if (bundleId) { if (bundleId in bundleIdCache) { throw "LoadJS"; } else { bundleIdCache[bundleId] = true; } }
Source code: https://github.com/kubetail-org/loadjs/blob/main/src/loadjs.js#L248
props are not hanging anymore, but we still need to figure why the Card Item doesn't get updated (Preview)
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@boulaffasae Nice detective work! ๐๐
Root cause:
ajaxPageState
is missingwhen you open a 2nd Icon Card, it try to load the same JS libraries that were loaded before. This throw an error, because we have a function that filter the JS array and only load the new JS - and it send an empty string which break loadjs
This points to us missing
ajaxPageState
support (\Drupal\Core\StackMiddleware\AjaxPageState
) in theprocessResponseAssets
function that was added in ๐ Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Fixed . That's how Drupal's\Drupal\Core\Render\MainContent\AjaxRenderer
and others handles that, and theajaxPageState
processing happens in\Drupal\Core\Asset\AssetResolver
.Alternative solution
But โฆ your alternative solution sure is intriguing โ that'd allow us to not send AJAX page state from the client to server and instead have potentially more cacheable responsesโฆ interesting! ๐
Next steps
- ๐ We should be able to reproduce this using an end-to-end test. Would you be willing to write that, @boulaffasae? ๐
- @bnjmnm, WDYT about @boulaffasae's alternative solution?
Hi @wim leers
I was able to reproduce the issue just by inserting one Icon Card in the existing Two Columns.
I added a simple test, that only check if clicking on that Icon Card will show the right props form / Currently, it show the Image props form instead of the Icon Card props form
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Interesting. This was a hack to get around XB not loading the themes libraries (including at top of each component). But maybe that issue was resolved? I can try removing the base.css from the card and see how it looks
Hi @wim leers
I tried getting the libraries and running UrlHelper::compressQueryParameter to compress them so we can attach them to the response:
if ($libraries = $assets->getLibraries()) { $libraries = UrlHelper::compressQueryParameter(implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($libraries))); $response->headers->set('Attach-Libraries', $libraries); }
What do you think ? We should decompress them on the processResponseAssets.ts level, do you think this is possible ?
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
I had assumed this was handling the libraries?
๐ The component preview should have a background: include theme's global asset libraries for component preview Needs work
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Fyi, I have removed the temporary extra base.css imports from each component but the hanging props form still remains but I'll let you'll with bigger brains figure that out :D
๐ Remove base.css import for all SDDS components Active
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Oh! I missed that you fixed the textareas and the image issue!!!
I just tried this MR branch and both of those work!!!
The image issue is here:
๐ Redux support for ImageWidget: `[image] String value found, but an object is required` Postponed
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
Note that the hanging doesn't happen with the MR branch but it doesn't show the props form now instead... but you are still working on things so not unexpected.
- ๐ธ๐ชSweden johnwebdev
#16 UrlHelper::compressQueryParameter uses zlib (gzcompress) which is not supported in the browser, so the frontend would also need to install a dependency like https://github.com/imaya/zlib.js/
That approach should be able to solve ๐ Make Media Library widget work when JS aggregation is enabled Active as well...
Hi @wim leers
I think we will run into a missing piece even after decompressing the libraries / the output will have names likes: core/ajax, ...etc
Redux might not understand those entries.
Can you please direct me into the correct ghin to do ^^
Some context //
This issue is not related to redux (redux show the spinner, because the ajax.js stuck)
When you open a component contextual panel, it try to load JS/CSS dependencies (dialog.js, ...etc)
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)- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
thanks for the thoughtsโฆ. this is my biggest blocker at the moment
No idea how to fix but hopefully yโall can squash it soon ๐ค
- Status changed to Needs work
2 months ago 8:31am 12 September 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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)- We must manually invoke
Drupal.attachBehaviors()
โ that's fine! :) That's how Drupal's AJAX system works too. - ๐
- This should work fine. That's how Drupal's AJAX system works too.
1+3 are actually already happening โ see
Drupal.behaviors.jsxAjaxProcess()
inajax.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. - We must manually invoke
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Note: this will likely also solve ๐ Make Media Library widget work when JS aggregation is enabled Active .
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())
- ๐ซ๐ฎFinland lauriii Finland
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.
- First commit to issue fork.
- Merge request !306Issue #3472900 by boulaffasae, kristen pol: XBEndpointRenderer &... โ (Closed) created by longwave
- Status changed to Needs review
2 months ago 2:06pm 12 September 2024 - ๐ฌ๐งUnited Kingdom longwave UK
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, andloadjs()
does not resolve the promise when asked to do nothing. The fix is to filter the list of libraries *before* trying to send it toadd_js
.I haven't yet got the Cypress tests working locally so I cherry-picked the new test from the previous branch.
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)
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
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
#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.
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
I'll try the steps in #35 first.
- ๐ฌ๐งUnited Kingdom longwave UK
@kristen pol
- Install Experience Builder, Media, Media Library
- On /xb/node/1 add a second Image component to the page
- Click on one of the image components, the form will work
- 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 tonpm run build
andddev drush cr
after switching branches. - ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
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! ๐ ๐ ๐
- ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
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
- Status changed to Needs work
2 months ago 9:55am 13 September 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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.
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)
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, ]); }
- ๐ฌ๐งUnited Kingdom longwave UK
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".
- Status changed to Needs review
2 months ago 6:05pm 13 September 2024 - ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
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.
- Status changed to Needs work
2 months ago 6:05pm 13 September 2024 - ๐บ๐ธUnited States Kristen Pol Santa Cruz, CA, USA
I didn't mean to change the status. Not sure what happened there.
- ๐ฌ๐งUnited Kingdom longwave UK
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.
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)
- Assigned to bnjmnm
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@bnjmnm and I pair-reviewed this to determine next steps.
He's going to combine the excellent work in both MRs (๐ @boulaffasae especially! ๐).
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
bnjmnm โ changed the visibility of the branch 3472900-fix-empty-js to hidden.
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
bnjmnm โ changed the visibility of the branch 0.x to hidden.
- Merge request !314#3472900: XBEndpointRenderer & processResponseAssets() do not support `ajaxPageState` โ duplicate CSS/JS loading โ (Merged) created by bnjmnm
- Issue was unassigned.
- Status changed to Needs review
2 months ago 2:45pm 17 September 2024 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
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.
- Assigned to bnjmnm
- Status changed to Needs work
2 months ago 3:26pm 17 September 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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 ๐ค
- Issue was unassigned.
- Status changed to Needs review
2 months ago 4:25pm 17 September 2024 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
Feeback addressed. Left threads open for anything with an explanation that might require clarification.
- Status changed to RTBC
2 months ago 4:37pm 17 September 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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 ๐
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
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 โ
committed 8ce73188 on 0.x authored by
bnjmnm โ
Issue #3472900 by boulaffasae, bnjmnm, longwave, wim leers, kristen pol...
-
wim leers โ
committed 8ce73188 on 0.x authored by
bnjmnm โ
- Status changed to Fixed
2 months ago 8:52am 18 September 2024 Automatically closed - issue fixed for 2 weeks with no activity.