๐ Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active is in, let's do this now!
drush cr
alone is insufficient, you need to reinstall (because default config changed). ๐
IOW: always doing the steps at the top of /CONTRIBUTING.md
is your safest bet.
๐
Remove the default value for the XB field and move components to a separate module
Active
will simplify those steps ๐
The distraction/off-topic "CI allowed to fail" one mentioned in #3 is now fixed: #3472316-11: CI: remove `phpunit.image` override thanks to upstream fix โ .
Is this more accurate, @jessebaker?
Oops! See #3472579-3: Tests are sometimes failing โ , this is actually introducing CI warnings, so it's a regression. This needs more attention.
I'm confused why it was reported as 100% green here on d.o now ๐ฌ
The warning started after ๐ CI: remove `phpunit.image` override thanks to upstream fix Fixed was merged, very suspicious.
Ahhhh โฆ it looks like https://git.drupalcode.org/project/experience_builder/-/pipelines/275317 failed, I merged that prematurely! Revertingโฆ
๐ Adding a component with slots does not register the slots as children Fixed landed ๐ข
Verifying fundamentals
For the experience_builder:heading
SDC, the generated Component
config entity does contain:
style:
field_type: list_string
field_storage_settings:
allowed_values:
-
value: primary
label: primary
-
value: secondary
label: secondary
field_instance_settings: { }
field_widget: options_select
default_value:
value: primary
IOW: primary
is selected as the default automatically, because that's the first example. So why is this not being respected? ๐ค
Turns out it is:
So the default
aspect mentioned in #12 is an irrelevant distraction.
So: server allows selecting "nothing", but how to make client know this?
All that matters is what @lauriiii was getting at in #10: if an SDC marks a prop as optional, we should be able to detect that (the "no value entered" scenario). So how do we do that?
Turns out that form_select_options()
hardcodes a particular special value: _none
โ specifically for <select>
. (Even the "list" field type's widget base class must hardcode this knowledge, see \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase::validateElement()
). It can optionally be overridden using #empty_value
.
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:
- the UI should interpret this as "no input specified by user"
- 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 - that will then prevent this invalid model to be sent from the client to the server:
That was simpler than expected! ๐
๐ Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active was just fixed.
The two other blockers are in progress too: ๐ Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work is being worked on by @bnjmnm, ๐ Adding a component with slots does not register the slots as children Fixed is being worked on by @balintbrews & @jessebaker.
Issue summary updated for you, @kristen pol ๐
Docs updated. Nits addressed. GIF added to issue summary.
Let's ship this! ๐
Thanks, @tedbow!
#7: that's
๐
Redux support for ImageWidget: `[image] String value found, but an object is required`
Needs work
, not this issue โ because you tested using ImageWidget
, not MediaLibraryWidget
.
This blocks ๐ [PP-2] Add ComponentAuditabilityTest Active .
๐ Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed landed weeks ago.
I think that now it makes sense to wait for ๐ [PP-2] Add ComponentAuditabilityTest Active , which will introduce part of what this aims to address: the beginnings of a UI that allows auditing what components are available in XB or not, and if not: why not.
๐ Mitigate class pollution from injected CSS Needs review is in.
Let's wait for @bnjmnm to reach a conclusion in ๐ฑ [META] Redux sync on ALL prop types, not just ones with a single [value] property Active first.
So that is why that stupid error re-appeared! ๐คช
I knew we fixed it in ๐ Tweak PHPStan rules Fixed , so I did not understand why it re-appeared โ I overlooked that path! ๐
Thank you!
Oops โฆ ๐
I got this wrong, because I got #3469609 wrong โ see #3469609-18: Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs โ .
Fixed now.
I do totally believe in you! ๐
Also, I very much messed up the sample commits and issue summary ๐
Issue summary fixed, and mistake rectified on the MR: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2....
๐ Adding a component with slots does not show the slots in the layers view Closed: duplicate was marked as a duplicate of ๐ Adding a component with slots does not register the slots as children Fixed by Jesse.
โจ Allow deleting component instances by pressing "Delete" or "Backspace" Needs review landed ๐ข
+1 โ I just didn't know you wanted to be involved like that โ definitely not back in May! ๐
Another issue that is closely related to this: ๐ Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work .
I think the solution that
๐
Media Library integration (includes introducing a new main content renderer/`_wrapper_format`)
Needs work
introduced with data-media-file
is not scalable/sustainable. But I do think there also is an opportunity to solve this generically.
See https://git.drupalcode.org/issue/experience_builder-3463842/-/tree/34638... for an outline (explained in 3 commits) of what I have in mind ๐ค (Although that surely won't work for real-time updates, only for AJAX updates, which is the case for complex widgets like image, media library, etc.)
Note: AFAICT this will need to call ImageItem::preSave()
for it to work, which is called by massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation()
. But that is only called upon actually saving an entity, and ever since
๐
Unable to save node article form โ remove obsolete TwoTerribleTextAreasWidget + fix duplicate `XB:image` SDC
Fixed
, that's not called at all anymore.
I figured out the root cause: this happens when uploading a new image using the ImageWidget
.
There simply is no Redux support yet for ImageWidget
. That's what
๐ฑ
[META] Redux sync on ALL prop types, not just ones with a single [value] property
Active
is for.
As of yesterday,
๐
Media Library integration (includes introducing a new main content renderer/`_wrapper_format`)
Needs work
landed and introduced hardcoded special support for MediaLibraryWidget
, with experience_builder_preprocess_media_library_item__widget()
generating a data-media-file
attribute containing the updated field properties.
Phased solution:
- IMMEDIATE WORK-AROUND: Just install the Media Library module and use the Media Library Widget ๐
- MR here: introduce similar behavior for
ImageWidget
, and piggy-back on what #3454173 introduced for Media. - Long-term: generic solution in ๐ฑ [META] Redux sync on ALL prop types, not just ones with a single [value] property Active .
MR incoming.
Tests aren't passing.
๐ Please don't mark an issue as needing review until the MR passes tests!
#8:
- Possibly. Let's wait for there to be 3 occurrences, to avoid premature abstraction. For now, please add an
@see
to the other place. - Yes ๐
So close! ๐คฉ
Great work here ๐
Figured out the root cause while pairing with @tedbow!
https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... is not yet addressed.
Nor is the rest of #15.
wim leers โ created an issue.
AFAICT tests are present and passing now ๐
@jessebaker has pointed out an oversight over at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2..., so marking .
In there, he also points out a scenario that is currently broken and that the test coverage should be expanded for.
Finally: please include a GIF showing this in action.
This must happen after ๐ Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active at least, and possibly more issues, to minimize disruption for in-progress MRs.
Almost ready! ๐
Thanks, @akhil babu! ๐
@lauriii was being trolled by Google Chrome โ he literally couldn't type anything anymore ๐ So, making this change on his behalf ๐
One post-review thought: we need to verify that the state
service remains in sync with the SDC discovery cache. Otherwise, we risk that the state key-value pair is deleted while the SDCs already are discovered. That'd result in zero incompatible SDCs getting listed, while we know that's not the case.
Definitely on the right track! ๐คฉ๐
At this stage I'm assuming it's debug css that has been accidentally committed. If it's meant to be there we can add it back into the hero but properly scoped ๐
๐
@jessebaker was just pointing that out!
Reproduced the last failure:
Drupal\Core\Render\Component\Exception\InvalidComponentException: [image.src] Does not match the regex pattern ^(/|http).*\.(png|gif|jpg|jpeg|webp)(\?.*)?(#.*)?$ in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).
Investigating ๐ต๏ธ
This stopped failing!
- #2885413 was committed on Aug 24: #2885413-157: Timestamp field items are affected by 2038 bug โ
- Then a follow-up commit happened on Aug 28: #2885413-164: Timestamp field items are affected by 2038 bug โ that changed the behavior
- โฆ and again later that day: #2885413-170: Timestamp field items are affected by 2038 bug โ
- โฆ and then it was reverted: #2885413-177: Timestamp field items are affected by 2038 bug โ .
So, postponing that on the upstream issue landing.
FYI, per ๐ [PP-1] Can't toggle boolean prop back to true after changing to false Postponed , @kristen pol created these related issues, which I suspect ought to be child issues of this one:
I already reported this over at #3463842-6: [META] Redux sync on ALL prop types, not just ones with a single [value] property โ , where @bnjmnm is devising next steps. Linking this there.
Thanks for creating this, @danielveza! ๐
There is an accumulating overhead for every additional event subscriber. This affects 100% of requests/responses, and if itโs only for architectural purity, then itโs not worth the overhead IMHO. But I don't feel strongly about this, plus at this early stage, that's essentially premature optimization.
So, deferring to the owner of this part of the codebase, @traviscarden, and RTBC'ing ๐๐
๐ Thanks, @balintbrews!
Needs a new round of feedback from @lauriii.
@danielveza That's a relief! ๐ฎโ๐จ
I'd like to avoid future devs having pain when trying to track down what is most likely a quite annoying performance regression to figure out.
That's totally fair.
Probably the softer approach for XB rather than the feature flag that removes it is a hook_requirements implementation that runs if JOSN:API is also enabled. I'll open a ticket.
I was going to suggest docs but โฆ this is far better! ๐๐คฉ Thank you! ๐
Precise STR would help ๐ Better yet, when you reproduce it, look in the network inspector at the request body for the last request to /api/preview/โฆ
. Find the entry for image
and you'll see exactly what the value is that is being sent for it. That'll determine where the root cause lies: back or front end.
We can make the page hierarchy (or layers) feel more robust by adjusting the styles in a way where elements are not moving around between state changes. We should consider both focus / active styles, as well as drag and drop.
Is this enough information for you to act on this, @balintbrews or @jessebaker?
First unbroke the MR, by resolving a conflict: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
Then paired with @bnjmnm, to figure out why things were failing in the build for that commit. Turns out that we're hitting a new edge case for input--SOMETHING.html.twig
. We discussed and agreed that it'd make more sense for those two files to be omitted and deferred to follow-up issues that are more tightly scoped โ they'll be child issues of
๐ฑ
[META] Redux sync on ALL prop types, not just ones with a single [value] property
Active
.
I trust @bnjmnm's judgment which parts of this MR to land and which parts are more reasonable and feasible to land in a follow-up :)
Great catch/call about uri
vs uri-reference
, @tedbow!
๐ Also validate request bodies against the OpenAPI spec Downport landed. I don't see how this particular case could occur anymore.
(Not a blocker anymore thanks to @bnjmnm's analysis on #3455975.)
This reminds me conceptually of ๐ Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() Fixed โ I wonder if we can do something similar here? Any inappropriate "wait" would then explicitly fail.
๐ฅณ โ thank you, much appreciated, @omkar-pd! ๐
@q0rban: Yay, thank you! ๐
We currently do this manually when developing XB:
# See it in action + recommended development environment
1. Drupal 11 (preferably a git clone, for git archeology โ 10.3 will work too).
2. `composer require drush/drush`
3. `drush si standard`
4. `drush pm:install experience_builder`
5. Browse to `/node/add/article` โ you'll see a `๐ช XB Demo โจ` field. Don't touch that โ just enter a title for the article and hit save: a component is rendered using the article title ๐ค
6. If you're curious: look at the code, step through it with a debugger, and join us!
7. If you want to run *all* tests locally, including the OpenAPI spec one: `composer require league/openapi-psr7-validator webflo/drupal-finder devizzent/cebe-php-openapi --dev`
โ /CONTRIBUTING.md
But we also have and end-to-end test that literally performs those steps, added months ago (
โจ
[MR Only] Edit any component prop, powered by a new FieldForComponentSuggester service, which will power the JS UI
Fixed
) in the super early XB days, but still serving its purpose: \Drupal\Tests\experience_builder\Functional\EndToEndDemoIntegrationTest::test()
And the Cypress end-to-end tests (similar to Nightwatch, but better) perform similar steps:
$field_definitions = \Drupal::service('entity_field.manager')->getFieldDefinitions('node', 'article');
$image_field_sample_value = ImageItem::generateSampleValue($field_definitions['field_hero']);
$node = Node::create([
'type' => 'article',
'title' => 'XB Needs This For The Time Being',
'field_hero' => $image_field_sample_value,
]);
$node->save();
โ \Drupal\Tests\experience_builder\TestSite\XBTestSetup::setup()
(@abhisekmazumdar in #4: look at that ๐)
Proposal: if you can make this work to the point where it installs the Standard install profile, creates the first article node (and perhaps fails for now) and then navigates to /node/1
, I'm happy to fill in the few missing pieces. ๐๐
๐ Retain the placement of components within the preview when inserting a component Active landed, let's land this next? ๐
๐ Retain the placement of components within the preview when inserting a component Active landed!
Is this unblocked now, or is it blocked on ๐ Improve UX of adding new sections Needs review ?
Assigning to @balintbrews to find out ๐
#21:
- @traviscarden has AFAIK started on ๐ OpenAPI validation errors must be provided as a JSON response Active ๐
- I cannot find this follow-up yet among https://www.drupal.org/project/issues/search/experience_builder?issue_ta... โ
- @traviscarden did this: ๐ Add TravisCarden to `CODEOWNERS` for OpenAPI Fixed ๐
WFM in general, with one exception:
/xb-api/v1/{resources} [ /{resourceId} [ /{subCollections} [ /{resourceId} ] ] ]
-1 to versioning the API at this time, because that implies it's a public API. It is not. Defining a public API for this that we provide BC for is definitely not in scope.
If you really want to have it, then let's use:
/xb-api/v0/{resources} [ /{resourceId} [ /{subCollections} [ /{resourceId} ] ] ]
(with v0
matching XB's major version: 0.x
)
P.S.: I do not understand why
We should also avoid
/xb/
if we're going to use that path for administrative UI routes.
matters? /xb-api/โฆ
vs /xb/api/โฆ
are essentially the same?
Lots of MRs landed, this now conflicts.
Fixed issues that should help y'all:
- ๐ Prop select lists don't affect the component Fixed is in
- ๐ Component config entities are incomplete: missing entries for optional props, causing errors in ComponentPropsForm Needs work is in ( ๐ Surface the REASON for an SDC not being made available in XB (i.e. not meeting criteria) Active is being worked on ๐
- ๐ Improve the page hierarchy display Active now visualizes the hierarchy better.
-
โจ
Allow components to use textarea in favor of input
Needs review
landed, which means you can now express a shape for your SDC's props to cause a
<textarea>
to show up โ but it won't work just yet โ see #3463842-6: [META] Redux sync on ALL prop types, not just ones with a single [value] property โ
โจ Implement the concept of sections within the client Fixed landed ๐ข
Great! TIL about https://www.drupal.org/project/layout_builder_iframe_modal โ ๐
Then the next steps are pretty clear โ updated issue summary accordingly.
Assigning to @bnjmnm for review/confirmation of @lauriii's proposal.
Thanks!
Can you please update \Drupal\Tests\experience_builder\TestSite\XBTestSetup
to make the same "test with an image that contains a space" changes that https://git.drupalcode.org/project/experience_builder/-/merge_requests/189 introduced for
๐
`{type: string, format: uri}` disallows image URLs containing spaces, `uri` data type stores + returns invalid URIs!
Fixed
? ๐
That should be able to reproduce it then!
Ideally we could outsource the styling completely to the admin theme so that we don't have to own this within the XB.
I don't see how this is realistic/feasible, because those admin themes will understandably style dialogs on the back-end only, not in the front-end (system.theme:default
) theme.
That's an unsolved problem in Drupal core too: #2195695: Admin UIs on the front-end are difficult to theme โ .
I don't understand why are we loading the media library within the XB application instead of loading it inside an iframe that would have all the required styles from the admin theme?
No idea โ that's the part of #3454173 that @bnjmnm handled, so assigning to him for answering that :) I suspect it's simply "because getting it to work at all was the only goal for #3454173" ๐ค
๐๐ข
FYI: @f.mazeikis landed ๐ Component config entities are incomplete: missing entries for optional props, causing errors in ComponentPropsForm Needs work earlier today, he's now pushing this forward ๐
#6.4 WFM!
Do you agree with
Can we get started with a E2E test that fails on failing to find the
required
HTML attribute of thetest_REQUIRED_string
prop on theall-props
test component? ๐
?
The back-end side LGTM now :)
AFAICT
_Nice to have:_ Refactor `layoutModelSlice.ts::replaceUUIDsAndUpdateModel()` and `layoutModelSlice.ts::insertMultipleNodes()`, make them unit testable, then write unit tests
is not yet addressed.
That's fine: it's a nice-to-have. Can we extract a follow-up issue for that, and have a less experienced team member take that on? ๐
@jessebaker reported the same bug at ๐ It should not be possible to drag a component from one preview into the other Closed: duplicate . Closed that as a duplicate.
This is a duplicate of ๐ Block dragging components from the desktop view to the mobile view Needs review .
Transferring credit.
๐๐
๐ Create a component for testing form backend + frontend integration Active landed weeks ago ๐
@bnjmnm, FYI: this has been added to
๐ฑ
Milestone 0.1.0: Experience Builder Demo
Active
not to solve all the things, but to address at minimum
โจ
Allow components to use textarea in favor of input
Needs review
โ and perhaps also type: boolean
(the latter is AFAICT a blocker in
๐
Component props form: map textarea, bool, and select elements to React components
Fixed
being reported by Utkarsh).
(i.e. make at least those 2 field widgets work, just like
๐
Prop select lists don't affect the component
Fixed
made <select>
work.)
It makes sense to me to prioritize this issue now, so that @bnjmnm can convert it to a meta issue where he outlines his plan for scaling this to all Drupal core field widgets.
With ๐ Prop select lists don't affect the component Fixed in, I'm now 100% confident this works.
So writing a passing tests should be easy.
One unaddressed nit, and that last commit introduced one more ๐
Merged in upstream, looks like more tests than before are failing.