✅ Unblocked by #3452397: Allow specifying default props values when opting an SDC in for XB.
✅ Unblocked on #3452397: Allow specifying default props values when opting an SDC in for XB.
✅ Un-soft-blocked by #3455898-4: Connect client & server, with zero changes to client (UI): rough working endpoints that mimic the UI's mocks, which may be fixed either there by @jessebaker or in a follow-up issue.
✅ Unblocked by #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings.
The UI currently assumes all SDC props have static values (which is why #3455898: Connect client & server, with zero changes to client (UI): rough working endpoints that mimic the UI's mocks's logic is even transforming SDC props populated by DynamicPropSources to static values for the client/UI, because that is what the client currently expects).
The HTTP API should race ahead of the UI, to empower the UI to surface multiple choices to the end user — choices that the FieldForComponentSuggester service can already generate:
types choices returned by that should be StaticPropSource computed by #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings.
instances choices returned by that should be presented as-is: the 'the "ALL PROPS" test component' test case in FieldForComponentSuggesterTest proves that this is already working well as-is. (We may need refinement in the ordering though — but already only viable choices are being presented 👍)adapter choices should be omitted for now, that is out of scope for #3454094: Milestone 0.1.0: Experience Builder Demo.This SHOULD also update the Open API spec if #3450308: Create an Open API spec for the current mock HTTP responses landed before this is done.
(See how in the /node/1/edit UI, the component instance with fake UUID dynamic-static-card2df uses a DynamicPropSource for the text SDC prop. Then see how in the UI, that is presented as a static value, i.e. as a StaticPropSource.)
/xb-component/… return not just {id: …, name: …, metadata: …} for every component, but {id: …, name: …, metadata: …, prop_source_choices: …}
FieldForComponentSuggester::suggest($component_id, EntityDataDefinition::create('node', 'article');adapters key that ::suggest() returns MUST be removed: out of scope for 0.1.0
types key MUST be instances key MUST be passed through as-is.Functional test coverage. Look at \Drupal\Tests\experience_builder\Kernel\FieldForComponentSuggesterTest + \Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest for inspiration.
All points above.
None — this is about leaping ahead of the UI, to allow it to offer the user a choice.
New API/endpoint to talk to.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | Screenshot 2024-08-29 at 8.20.54 AM.png | 559.62 KB | bnjmnm |
| #27 | Screenshot 2024-08-28 at 3.27.09 PM.png | 101.63 KB | bnjmnm |
| edit-component-props-always-static.gif | 3.03 MB | wim leers |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
wim leersComment #3
larowlanThe FE of this is handled in #3452582: Make store API paths/uris configurable via environment variables (and eventually drupalSettings) it shows (in comments) the extension points. I knew we'd need this at some point so took the time to document it there.
Comment #4
wim leersI need to update this issue — many things have changed since this was created.
Comment #5
wim leersAll originally identified blockers are in. But #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings changes things.
Issue summary fully updated 👍
Comment #6
wim leers#3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings is in.
Comment #7
wim leersComment #8
bnjmnmComment #10
bnjmnmComment #11
wim leersThank you, @bnjmnm, for getting this going! 🤩🙏
This looks stellar. Zero remarks on the code. Looks like the details I write >2 months ago were sufficient? 🤓
The exact shape of the response is irrelevant at this time — because the very purpose of this issue is to just leap ahead of the UI and at least allow some experimentation to happen on the front end when the necessary designs for this eventually arrive.
But I am surprised the new test does not fail because #3450308: Create an Open API spec for the current mock HTTP responses introduced an OpenAPI spec that covers this route too. In that OpenAPI spec,
/xb-componentshas this schema:and that
$refdoes listadditionalProperties: false, which this test should be triggering? 🤔Now, I know that I literally just wrote:
… but I'd expect we'd need to update
openapi.ymlsomewhat like this as a very MVP way to capture this in the OpenAPI spec:Comment #12
wim leersStill missing here: an update to
openapi.yml. The fact that this is currently passing tests is indicative of theopenapi.ymlfile not being validated correctly byApiResponseValidator, becauseadditionalProperties: falsein$ref: '#/components/schemas/Component'should result in a validation error 😬Comment #13
wim leersComment #14
wim leersUgh, looks like
thephpleague/openapi-psr7-validatordoes not really supportpatternProperties, because the only mention of it is in a comment, and the same goes fordevizzent/cebe-php-openapiwhich it relies upon?! 😱🤯😭
Time to call it a day.
Comment #16
tedbow@Wim Leers see my MR comments. let me know what you think the `x-xb-validation` using the OpenAPI extension naming to check this. If you are good with approach assign it back to me and I will finish it up
Comment #17
wim leers+1 for this direction. Details on the MR. Glad you've found a way forward! 👏
Comment #18
tedbow@Wim Leers see my MR comments
Comment #19
tedbowActually I still need to look at test failures
Comment #20
tedbowUnassigning myself. I am not sure the Cypress test is failing. I have the tests running locally but I can't figure it out. Thought maybe it was `ApiResponseValidator` but I commented that out and the test is still failing
Comment #21
balintbrews#3463307: Implement simplified zoom interface significantly rewrites those end-to-end tests and introduces hardening for the tests to avoid trying to access elements in the iframe before their events are initialized. It's close to being merged, so I recommend that we wait until it lands, and see how the failing tests behave with those changes.
Comment #22
wim leersThanks, @balintbrews!
Comment #23
tedbow@balintbrews thanks!
Comment #24
wim leersTurns out it's not #3463307: Implement simplified zoom interface that will fix that, but #3470490: Unnecessary wait() use in e2e tests can cause failures and misleading fail output.
Comment #25
wim leersComment #26
wim leersComment #27
bnjmnmThere's a remaining test failing because it is failing to find an iframe where
data-test-xb-content-initializedisTRUEand I discovered why by addingconsole.log()calls next to wheredata-test-xb-content-initializedis switched to false and true.The two final logs - both which change the attribute to FALSE (without the expected switch to TRUE) - only happen in this MR.
I can see how this might happen since the switch to FALSE will occur any time the useEffect is triggered by a change to
dispatch, height, handleDragAdd, handleDragEnd, handleDragStart, layout, model, components,but the switch to TRUE only happens when theonloadcallback is invoked. It seems like there are plenty of opportunities for one of the 8 useEffect dependencies to change and changedata-test-xb-content-initializedto FALSE withoutonloadfiring to change it to TRUE.If those extra renders of the iframe are unnecessary, perhaps they should be addressed in this issue. If it is expected and acceptable, then we need a different way to mark something as initialized.
WITH THIS MR OPEN YOU CAN CONFIRM THIS BY... going to the XB UI and look at the iframe html - you'll see the
data-test-xb-content-initializedwithin moments of the UI loading.Comment #28
bnjmnmFurther debugging shows that this MR is not creating any new changes that trigger the iFrame attribute update changes. Instead, it is changing the order that they change, and components is changing after layout and model.
A change to
componenttriggers the useEffect that setsdata-test-xb-content-initializedto false, but unlike layout/model it does not result in the iframe loading anything new, so theonloadthat would setdata-test-xb-content-initializedto TRUE is not invoked.Comment #29
bnjmnmBased on the extensive debugging it looks like this can be addressed by changing a few lines so
components(a list of the available components) is no longer a dependency of the iframe-impacting useEffect or theupdateDatauseCallback. Neither of these should need to update if the components list changes.While it's true that
updateDatarequirescomponentsto have initialized for it to properly add a component to the layout, this doesn't need to be enforced as a callback dependency because the UI doesn't offer that option until it is ready anyways.I think this solution is sound but it would be good to get another FE person to look it over to ensure this isn't going to break something elsewhere.
Comment #30
balintbrewsTruly great investigation, and I agree with the solution! I love how this ended up being an actual problem — unnecessary rerenders — our test uncovered.
Comment #31
wim leers👏 @bnjmnm! Agreed with @balintbrews 😊
Comment #33
wim leersComment #34
tedbowSomehow the removal of
patternPropertiesgot remove from this MR will open up a follow-up