Build Breadcrumb and SiteBranding code components

I tagged this as beta blocking because 🌱 Plan how to evolve code component overrides Active is, and this is part of that.

FYI: this is possible since 📌 Populate data to drupalSettings to enable Dynamic Code Components Active and 🐛 xbData.* libraries aren't reliably attached for use by the preview while editing a code component Active .

This will need to update \Drupal\experience_builder\CodeComponentDataProvider::getRequiredXbDataLibraries() to stop checking for _drupalSettings_xbData_v0_breadcrumbs and _drupalSettings_xbData_v0_branding, because it'll need to check for the presence of a call to ::getSiteData() and/or ::getPageData() instead.

This was agreed in 📌 Improve the way to detect if the code component has drupalSettings Active as an intermediate step, see #3533458-5: Change CodeComponentDataProvider::getRequiredXbDataLibraries() to base its logic on information provided by the front-end rather than on naive string/regex matching → for the proposed plan, +1'd by @balintbrews at #7 and by @larowlan at #9.

FYI: this is possible since 📌 Populate data to drupalSettings to enable Dynamic Code Components Active and 🐛 xbData.* libraries aren't reliably attached for use by the preview while editing a code component Active .

This will need to update \Drupal\experience_builder\CodeComponentDataProvider::getRequiredXbDataLibraries() to stop checking for _drupalSettings_xbData_v0_baseUrl and _drupalSettings_xbData_v0_pageTitle, because it'll need to check for the presence of a call to ::getSiteData() and/or ::getPageData() instead.

This was agreed in 📌 Improve the way to detect if the code component has drupalSettings Active as an intermediate step, see #3533458-5: Change CodeComponentDataProvider::getRequiredXbDataLibraries() to base its logic on information provided by the front-end rather than on naive string/regex matching → for the proposed plan, +1'd by @balintbrews at #7 and by @larowlan at #9.

balintbrews → credited justafish → .

I expanded the issue to cover page title as well. Also adding credits for discussions prior to this issue being created.

Pushed a test with each of these three components following lead in 📌 Build a navigation code component Active (and on top of that).

Changes for this branch only are in this commit

@balintbrews merged 📌 Build a navigation code component Active 🥳, the MR here now needs rebasing on top of that.

Also, #3 has not yet been addressed. Without that, this cannot work reliably.

I recommend starting with updating tests/fixtures/recipes/test_site/config/experience_builder.component.js.xb_test_code_components_using_drupalsettings.yml, which should then cause the tests to fail, which will force updating getRequiredXbDataLibraries().

Don't make the components use drupalSettings.xbData... directly. Instead, create API functions getPageData() (for breadcrumb and page title) and getSiteData (for site name, logo, home URL, site slogan, and base URL) within @/lib/utils or similar.

Ah sorry, I was following the lead of the navigation one

x-post

Going to assume everyone else is asleep and try and move the needle on this

This is ready for another look 🎅🚲️🎄

If we are not using useSWR, the XB date fetch feature doesn't work with this. Not sure what to do about that – it's not great that some of our utilities are using it and others are not.

For some reason, this wasn't working outside of XB. It looks like the values are null in drupalSettings. Has anyone else verified this works outside of XB?

That sounds like a problem related to #13 — I see the MR changed ::getRequiredXbDataLibraries(), but perhaps incorrectly. Checking 🕵️

#18:

If we are not using useSWR, the XB date fetch feature doesn't work with this. Not sure what to do about that – it's not great that some of our utilities are using it and others are not.

That's a really good point. Somehow I've been thinking that we'd use useSWR only for HTTP requests, which is its primary purpose, but probably nothing stops us from using it for reaching into drupalSettings. Maybe if the new API methods returned promises, it would be all we need?

Debugged, and @larowlan's updated ::getRequiredXbDataLibraries() logic works fine. (But we should really still do #3533458-12: [PP-1] Change CodeComponentDataProvider::getRequiredXbDataLibraries() to base its logic on information provided by the front-end rather than on naive string/regex matching → — not here/now though.)

Root cause

Found the culprit, and it's 2-fold, both missed bugs from 📌 Populate data to drupalSettings to enable Dynamic Code Components Active :

1. We use hook_js_settings_build() (whose results are cached), but should have used hook_js_settings_alter() (whose results are not cached). A simple mistake. Not worth adding test coverage for IMHO.
2. The \Drupal\experience_builder\CodeComponentDataProvider::getPartialXbDataFromSettingsV0() logic was using isset() … which has very weird/special behavior when it comes to key-value pairs where the value is NULL … 😅 Switching to array_key_exists() is all we need. PHP--

Once again: never use isset(), and always do exact assertions of the full data structure expected. I was less strict while reviewing that one in order to keep things moving and it looked simple/innocent enough, and look, it bit us. 😬

Solution

1. Expand test coverage: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... → failed 👍
2. Fix: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Final sign-off from @balintbrews now, because the back-end pieces are ready (and approved) 👍

Spoke too soon; there's a regression, probably because Drupal's asset library system calls hook_js_settings_build() and hook_js_settings_alter() at different times 😬

Green again for @balintbrews 👍 This is a bit of kinda tricky logic, but it's worth it: the asset library definnitions in *.libraries.yml show an outline of what to expect (matching best practices in Drupal core), it supports overrides, and it loads only what's necessary (which is currently all tiny but may not remain the case) hence paving the path for #3533458-12: [PP-1] Change CodeComponentDataProvider::getRequiredXbDataLibraries() to base its logic on information provided by the front-end rather than on naive string/regex matching → .

The last bit I discovered is that 📌 Populate data to drupalSettings to enable Dynamic Code Components Active 's interpretation of baseUrl is an absolute URL, without the subdirectory. That's not how drupalSettings.path.baseUrl works … so I switched to that. Then I realized it was literally in the instructions/issue summary of 📌 Populate data to drupalSettings to enable Dynamic Code Components Active … so reverted it once again. So documented that confusing difference explicitly:

        // ⚠️ Not the same as `drupalSettings.path.baseUrl` nor Symfony'
        // definition.
        // @see \Symfony\Component\HttpFoundation\Request::getBaseUrl()
        // @see \Drupal\system\Hook\SystemHooks::jsSettingsAlter()

@lauriii, I put more thought into what you said about not using useSWR. While standardizing on that sounded sensible at first, I don't think it's the way to go. The main reason of providing useSWR is to ensure reactivity in the components while async data fetching is happening. That's not the case here, the data is synchronously available in the window object, we just need to return it. To make our utility functions work with useSWR we'd need to turn them into async functions with no good reason. Not to mention that we wouldn't make use of SWR's caching. I understand that the code editor's data explorer is tied to useSWR, but that data explorer was built to explore more complex data shapes and potentially larger amount of data, whereas the new API methods can be easily documented due their very simple data shapes. In case we miss the data explorer functionality, I propose we make that happen later as part of the API methods, as opposed to forcing component authors to wrap them in a React hook.

Assigning back to Wim to wrap up how baseUrl is populated.

That's not the case here, the data is synchronously available

💯

This would only not be true on sites that do not do "regular" page loads, i.e. sites using something like http://drupal.org/project/refreshless.

I've added data fetch support to the utility functions (without using useSWR)

→

Nice!!! 🤩

Discussed #24 with @balintbrews, turns out we do want root-relative base URLs, not absolute ones, and with a trailing slash.

Automatically closed - issue fixed for 2 weeks with no activity.