β οΈ This assumes that the auto-saved code component is working. If it doesn't, then it'd be the client-side equivalent of π Improve server-side error handling Active , which needs its own issue. Raised this at #3499919-20: [Meta] Plan for in-browser code components β .
π Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active is missing from the issue summary. Is that intentional?
It also doesn't account for the fact that auto-saves are not guaranteed to be in a working state. Which means that once π Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active is done, a temporarily broken code component could break all XB previews.
(For example: modifying the code to not output a slot, while the metadata does say that that slot exists.)
This is RTBC, but now conflicts with upstream. :) Almost!
SingleDirectoryComponentTest::testRewriteExampleUrl()
is being added in
π
Adding the Image component results in a state considered invalid
Active
.
π€©
Thank you! Excellent initiative π
wim leers β made their first commit to this issueβs fork.
#62 is actually what @lauriii expressed on the meeting I had with him as his ideal.
FYI: this blocks #3502769, see #3502769-4: [PP-2] auto-saved PageRegions may be invalid, but MUST NOT cause XB's previews to fail. β .
This blocks #3502769, see #3502769-4: [PP-2] auto-saved PageRegions may be invalid, but MUST NOT cause XB's previews to fail. β .
WRT PageTemplate
validation
π Page title should not be required Active landed, so #3 cannot happen anymore.
#3501600-17: Split 1 PageTemplate config entity into N PageRegion config entities β describes how we'll be able to drop most validation, probably all the validation that caused #3 in the first place.
So let's wait for π Consider refactoring page_template into page_region(s) Active to land, and then re-assess this.
Related, at a different level
This is level up from π Improve server-side error handling Active , but is comparable in intent. A "level up" in the sense that that is about individual component instances, and this is about a tree of component instances.
So let's wait for π Improve server-side error handling Active to land too, and then re-assess this.
- I approved the BE in #5.
- @hooroomoo did manual testing in #7 and found 2 things.
- @bnjmnm dug into #7 and found it was due to π Exapand Media Library admin theme rendering beyond props form. Active not having been fixed yet. That has now been fixed!
- @jessebaker did a review of the FE code at https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
This improves on the status quo. Let's get it in.
wim leers β made their first commit to this issueβs fork.
Consult how this change needs to be represented in the code component's config entity
The way this was implemented in β¨ Config entity for storing code components Active + β¨ Create a ComponentSource plugin for JS components Active :
- The "code component" (
JavaScriptComponent
config entity) has itsstatus
set totrue
. - This causes
\Drupal\experience_builder\EntityHandlers\JavascriptComponentStorage::doPostSave()
to create the correspondingComponent
config entity.
This is already implemented and tested on the back end.
So the UI shown in the issue summary should only need to PATCH
the config entity (using the /xb/api/config/js_component/β¦
) and set status
to true
.
Now all this needs is a nice screencast or GIF ππ
wim leers β changed the visibility of the branch 3485692-create-extendibility-proof to hidden.
wim leers β changed the visibility of the branch extension-and-example-try-vite to hidden.
On it!
I had 2 remarks: docs (Jesse added those π) and a @longwave review of some clever bypassing of a core API. @longwave +1'd it for here, but did confirm the need for a follow-up, which he created: π Refactor library discovery for extensions Active . And seems like @effulgentsia implicitly agreed with that too in #13 :)
What do you mean? π€
Limiting to XB's own Page
is IMHO dangerous because it doesn't expose us to the wider reality. Which means we'll end up with known unknowns and unknown unknowns.
Furthermore, the status badge ( β¨ The status badge should indicate if there are changes to the page Active ) would otherwise fail to work correctly for article nodes.
This is what @lauriii indicated in DM.
You left it in a great spot, was a pleasure! :)
The same problem exists for "article" nodes. Shouldn't that be fixed in the same way? That's also what I was referring to at #3498525-35: Allow XB to be used on all bundles of all revisionable, publishable content entity types β .
Added screenshot to show what it should look like instead.
I bet either:
- XB is messing up the markup somehow
- Olivero is expecting specific markup, including expectations of
BlockPageVariant
being used
The latter is easily confirmed:
const searchWideButtonSelector =
'[data-drupal-selector="block-search-wide-button"]';
const searchWideButton = document.querySelector(searchWideButtonSelector);
const searchWideWrapperSelector =
'[data-drupal-selector="block-search-wide-wrapper"]';
const searchWideWrapper = document.querySelector(searchWideWrapperSelector);
β core/themes/olivero/js/search.js
which is targeting markup literally only available in core/themes/olivero/templates/block/block--secondary-menu--plugin-id--search-form-block.html.twig
π
And that Twig template is specifically targeting a Block
config entity in the secondary_menu
region with using the search_form_block
plugin ID.
Conclusion
Olivero is heavily tightly coupled with the Block module. π±
I don't see how this is possible to solve without either:
- hardening Olivero
- duplicating a lot of Olivero inside XBβ¦ π±
Yes. π Fixed.
I assumed the different states you described in
#3505118-4: [PP-1] The status badge should indicate if there are changes to the page β
would not need to apply only to the Page
content entity type, but to all edited content entities? Is that wrong?
Also, can we link the designs in the issue summary? π
To clarify: I defer to @lauriii where to draw the line, but if not everything in #19 is addressed here, there will definitely need to be one or more follow-ups.
#16 seems reasonable to consider part of this issue from a UX POV, but I don't think it makes practical sense.
Because it would need to work generically, not just for the Page
content entity type. Plus, it'd require no longer hardcoding the link in the UI. Wrote up a detailed plan at
π
Disallow deleting an XB-enabled content entity if it's currently the homepage
Active
.
The UI doesn't currently convey:
- that the currently viewed page is the current homepage
- that the currently viewed page is about to become the current homepage (thanks to auto-saving)
- which page in the list is the current homepage
I also think that the UI strings are quite confusing, which @tedbow also pointed out in the MR.
This is what it all currently looks like:
@mglaman has indicated these aspects are missing from the design.
wim leers β created an issue.
Added STR for using article nodes.
Oops, overlooked the cspell
violation this introduced, fixing in
#3506280-5: Fix tests to handle validation message improvement in Drupal 11.1.2 β
.
I overlooked the cspell
violation that
π
Cannot redeclare hook_field_widget_info_alter()
Active
introduced π Let's fix that here too.
I believe this is indeed the correct place/approach, but it should not hardcode title[0][value]
.
See the MR for β¨ The status badge should indicate if there are changes to the page Active at https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... for the correct generalized approach, which looks like this:
$label_field_input_name = sprintf("%s[0][value]", $content_entity_type->getKey('label'));
$is_new = $this->contentEntityIsConsideredNew($entity_form_fields[$label_field_input_name], $content_entity_type);
π€― Shouldn't phpstan-drupal
be configured to not parse *.api.php
files? π€
FYI: escalated to @effulgentsia, to decide when to get time from Ben/BΓ‘lint to work on this problem space.
Debugged this.
#5 is inaccurate AFAICT.
Debugging trail
- Create a fresh
node
withstatus: false
. Say, node 4. - Load it in Experience Builder at
/xb/node/4/editor
. Observe that the toggle is toggled on. - Put a breakpoint in the code that generates this form:
\Drupal\experience_builder\Controller\EntityFormController::form()
. Observe this is the return value:
Most importantly:#value === FALSE
. - In
\Drupal\Core\Render\Element\Checkbox::preRenderCheckbox()
, that's still the case. - β οΈ However, that maps the
#return_value
Form API property to thevalue
attribute:
Element::setAttributes($element, ['id', 'name', '#return_value' => 'value']);
which results in
- Look at the corresponding FE code:
/ui/src/components/form/components/drupal/DrupalToggle.tsx
. It contains this:
<Toggle checked={!!attributes?.value}
β¦ which appears accurate but isn't. The
value
attribute for<input type="checkbox">
is very interesting π€ͺ, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox....So AFAICT the problem lies in
DrupalToggle
, which was introduced in π Split form components into `Drupal`-prefixed behavioral wrappers and presentational components Needs work .
Looking at /node/<nid>/edit
- checked "published" checkbox:
<input data-drupal-selector="edit-status-value" type="checkbox" id="edit-status-value" name="status[value]" value="1" class="form-checkbox form-boolean form-boolean--type-checkbox">
- unchecked "published" checkbox:
<input data-drupal-selector="edit-status-value" type="checkbox" id="edit-status-value" name="status[value]" value="1" checked="checked" class="form-checkbox form-boolean form-boolean--type-checkbox">
<input data-drupal-selector="edit-status-value" type="checkbox" id="edit-status-value" name="status[value]" value="1" class="form-checkbox form-boolean form-boolean--type-checkbox">
<input data-drupal-selector="edit-status-value" type="checkbox" id="edit-status-value" name="status[value]" value="1" checked="checked" class="form-checkbox form-boolean form-boolean--type-checkbox">
π That's the markup being generated outside of XB. That's the starting point for things we do on top.
Tentative conclusion: needs only front-end changes?
So AFAICT updating DrupalToggle
should work? But
- checked={!!attributes?.value}
+ checked={!!attributes?.checked}
didn't do the trick π
I'm getting lost between:
ui/src/components/form/components/Checkbox.tsx
ui/src/components/form/components/drupal/DrupalInput.tsx
ui/src/components/form/components/Toggle.tsx
ui/src/components/form/components/drupal/DrupalToggle.tsx
β¦ because all four of those (!!!) are dealing with the checked
attribute π
AFAICT only the last 2 are relevant. I changed both like indicated above, without success.
I've done the due diligence, and think it's now down to somebody who knows the Semi-Coupled theme engine well enough to finish it up. It probably takes them only minutes given the digging I've done so far π€
Forgot to signal this is soft-postponed for #9.2: there is a pure BE MR that can already land and should be reviewed.
But for the FE pieces to happen, both this issue's MR and the one at π Once previewed in XB an entity with no changes will still show up in "Review x changes" Active must have landed.
π€£ Ran into the infamous d.o bug where concurrent edits cause an issue to become unpublished! Oh irony π
#3: Regarding the odd "Published" badge behavior β : agreed. For that we have π "Published" checkbox is always checked even if the entity is not Active .
#4: No, that diagram does not address @jessebaker's concerns in the 2nd and 3rd paragraphs of #3. Because this diagram refers only to interactions/states at the "XB auto-saving" level. It does not address the confusion between XB's "Publish all changes" button and the "Published" checkbox on content entities that use \Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions()
.
@jessebaker in #6 devised (π€©π) a way to achieve those 6 XB UI states ("new content", "draft", "published", "changed", "deleted", "archived"), and uses the "Published" field (\Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions()
) in them. But that doesn't mean the UI confusion has been solved π
I think @jessebaker captured well in his 3 points in #6 what is still needed.
- A "new" content entity can only be created for
Page
, not forNode
or anything else. Thanks to β¨ Provide a way to create a new page Active . So a general solution for this point is not realistic at this time.(We cannot use
$entity->isNew()
, because that only works for a "new in PHP runtime" check, but here the entity has already been created.)However, we can deduce a reasonable heuristic already I think:
- a label is likely to always be required (and we could make that another requirement in π Allow XB to be used on all node types Active )
- which is why
\Drupal\experience_builder\Controller\ApiContentControllers::post()
generatesUntitled page
as the default - β¦ which is identical to
t('Untitled @singular_entity_type_label', $entity_type->getSingularLabel())
- So let's use that as the heuristic for now!
β Created a draft MR on this issue that adds this information to the
/api/layout/{entityTypeId}/{entityId}
API response, underisNew
. - This require
π
Once previewed in XB an entity with no changes will still show up in "Review x changes"
Active
to be fixed. I refined that earlier today, see details at
#3502902-6: Once previewed in XB, a content entity with no changes incorrectly appears in "Review x changes" β
.
β Postponing this issue on that one. - Since
β¨
Provide an API for listing available pages
Active
, this has been available for
Page
content entities. But here we need it for:- all content entity types supported by XB (not a generic solved problem, see π Allow XB to be used on all node types Active , where I summarized the discussion here in comment 33 and credited you both!)
- not for a list of content entities, but specifically for the currently edited content entity. This part is missing.
β Created a draft MR on this issue that adds this information to the
/api/layout/{entityTypeId}/{entityId}
API response, understatus
.
Conclusion
- The BE MR I just created can already land and would fix points 1+3.
- Point 2 would be fixed by β¨ Provide an API for listing available pages Active
- After both are in, the remainder of this would be a trivial pure front-end issue.
P.S.: AFAICT this is not blocked on π "Published" checkbox is always checked even if the entity is not Active , but it sure makes things confusing π
wim leers β made their first commit to this issueβs fork.
Failing tests due to recent upstream core changes. The failure is bogus.
Glad to see this done.
And nice diffstat too: +12, -54
π
Per @lauriii's diagram for "XB publishing states" at
#3502902-4: Once previewed in XB, a content entity with no changes incorrectly appears in "Review x changes" β
and @jessebaker's concrete interpretation of that in concrete content entity terms at
#3502902-6: Once previewed in XB, a content entity with no changes incorrectly appears in "Review x changes" β
, I think there's one more requirement: to be eligible, the content entity type must implement \Drupal\Core\Entity\EntityPublishedInterface
too.
Not 100% certain though. So asking @lauriii to confirm.
Yep, green! π₯³
That's why I also pushed the secondary clean-up bit (point 2 under "proposed resolution").
The MR at π Allow CMS Author to delete a page Postponed is 99% ready, and it does fix this too, and includes test coverage for it π
Approved the BE pieces of this MR. π
Still needed:
- failing tests, see #10
- the (trivial)
openapi.yml
feedback has not yet been addressed
The new e2e test is failing:
cy:command β findByLabelText Page options for Empty Page
cy:command β click
cy:command β contains div.rt-BaseMenuItem, Delete page
cy:command β click
cy:command β contains button, Delete page
cy:command β click
cy:fetch β GET http://localhost/web/session/token
Status: 200
cy:fetch β DELETE http://localhost/web/xb/api/content-delete/xb_page/2
Status: 204
cy:command β wait @deletePage
cy:fetch β GET http://localhost/web/xb/api/content/xb_page
Status: 200
AFAICT it really is as simple as #5.
Select tests pass locally. Hopefully the MR confirms it for the full test suite.
Isn't this just
Only do auto-saves for entities that are different than their saved (live) versions.
Alternatively: compute the data_hash
for the saved entity and check if it's different.
Having read \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable()
(which creates the auto-save entry for the edited content entity using client-side representation) and \Drupal\experience_builder\Controller\ApiLayoutController::get()
(which computes client-side representation to boot the XB UI), it AFAICT should be possible to just compute the hash, using something like:
diff --git a/src/AutoSave/AutoSaveManager.php b/src/AutoSave/AutoSaveManager.php
index 0cb1419c..322b93cb 100644
--- a/src/AutoSave/AutoSaveManager.php
+++ b/src/AutoSave/AutoSaveManager.php
@@ -5,6 +5,8 @@ namespace Drupal\experience_builder\AutoSave;
use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Entity\TranslatableInterface;
+use Drupal\experience_builder\Controller\ApiLayoutController;
+use Drupal\experience_builder\InternalXbFieldNameResolver;
/**
* Defines a class for storing and retrieving auto-save data.
@@ -27,6 +29,22 @@ class AutoSaveManager {
return $this->tempStoreFactory->get('experience_builder.auto_save', expire: $expire);
}
+ public function hash(EntityInterface $entity, ?array $data): string {
+ // Generate a hash for the saved entity.
+ if ($data === NULL) {
+ $field_name = InternalXbFieldNameResolver::getXbFieldName($entity);
+ $tree = $entity->get($field_name)->first();
+ $data = [
+ // @todo ::buildRegion is not currently callable, but *could* be.
+ 'layout' => ApiLayoutController::buildRegion('content', $tree, $model),
+ 'model' => $model,
+ // @todo ::getEntityData is not currently callable, but *could* be.
+ 'entity_form_fields' => ApiLayoutController::getEntityData(),
+ ];
+ }
+ return \hash('xxh64', \serialize($data));
+ }
+
public function save(EntityInterface $entity, array $data): void {
$key = $this->getAutoSaveKey($entity);
$auto_save_data = [
In \Drupal\experience_builder\Controller\ApiPublishAllController::__invoke
we have logic to convert auto-save data to entities ready for validating/saving. We may want to move this logic into the auto-save manager(or somewhere else) so we can then compare the entities to the saved version. If there no changes to the saved versions we would then not create the auto-save entry and if there already is one delete it.
AFAICT \Drupal\experience_builder\Controller\ApiPublishAllController::validateExpectedAutoSaves()
does not look at the saved data at all currently? So I don't think that logic helps.
So this needs to undo what \Drupal\Component\Utility\Html::escape()
did, interesting! π
This is 99% correct AFAICT, but this would make it 100%: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... β asking @longwave to confirm.
Reviewed the back-end bits.
Who should review the FE bits? Or, do you feel this needs no further review, @bnjmnm?
@lauriii opened π Page title should not be required Active for #10, and I fixed it.
@lauriii: I proposed to do something similar for the "messages" block over at #3501600-17: Split 1 PageTemplate config entity into N PageRegion config entities β .
Adding to #8: π Page title should not be required Active landed too, so the "validation across the component trees for all regions in a given theme" just became simpler.
To recap, there are 3 special kinds of blocks: "main content", "page title" and "messages".
- as of π Page title should not be required Active , the page title no longer gets special treatment β thanks to @lauriii's response to #3505872-5: Page title block should not be required β .
- per #8, the "main content" one no longer needs any special treatment either, at least not at the validation level. Because we'll hardcode that block being the sole one in the
content
region. - that leaves only "messages"! I personally think it merits getting almost the same treatment as the page title one, with one difference: if
\Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant
didn't encounter >=1 such block during rendering, then XB's page variant should just place it at the top of thecontent
region automatically, which is exactly whatBlockPageVariant
does:
// If no block displays status messages, still render them. if (!$messages_block_displayed) { $build['content']['messages'] = [ '#weight' => -1000, '#type' => 'status_messages', '#include_fallback' => TRUE, ]; }
If @lauriii agrees with that, then we'll have eliminated all of the validation challenges in this MR! It'll allow this MR to delete \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeMeetsRequirementsConstraint::$nested
and related code.
That test looks like a nice start! π π
#2: seems like the expected Workspace
config entity does not exist yet, since that NULL
can only come from
Workspace::load($id)
WFM! :D Go ahead and merge as far as I'm concerned :) But β¦ I think it'd be better/simpler to first get tests to pass on the current MR? π That keeps the focus on the most important thing?
Just walked @lauriii through what I did in #58 (at the end of a meeting about something else). He VERY much does not like the https://example.com
prefix π
He agrees with the overall approach, but just explicitly doesn't like that prefix.
I respectfully disagree, but given @lauriii has a much stronger front-end development past than I do, I defer to him π Done: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/457...
Re-testing again. Locally it passes. π¬
I believe I see a solution for
- agree upon the syntax for letting an SDC specify a default image, knowing it CANNOT be a resolvable image URL, and perform the necessary transformation to a resolvable image URL
in ~100 LoC (nutshell: require example values to start with https://example.com
, add new method to ComponentSource
to rewrite such example values to resolvable URLs):
The only thing that's still missing: updating \Drupal\experience_builder\ComponentMetadataRequirementsChecker::check()
to require examples
values for type: string, format: uri|iri|uri-reference|iri-reference
to start with https://example.com
.
See the second branch:
- making it work in ~100 LoC: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/3dc...
- getting rid of the auto-created Media entity for the
600x400.png
sample image: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/bf1...
P.S.: this also paves the path for examples for images to become optional if min/max dimensions are specified, then we could auto-generate a relevant sample image. See β¨ Feature request: Ability to specify minimum image resolution for an image upload Active .
I was thinking about this some more last night.
When and why do we need this FallbackPropSource
? We need it for:
preview purposes, never storage purposes
never for booleans, integers, or numbers
never for any string format
other than one of the uri
+ uri-reference
+ iri
+ iri-reference
ones: https://json-schema.org/understanding-json-schema/reference/type#format
β¦ and even then, only when they must be resolvable/fetchable, to make certain tags work: <img>
, <video>
β¦
So AFAICT these special needs do not apply to anything else. The most common "resolvable URL" case is links (<a href="<SDC PROP HERE">link text</a>
), but there it doesn't apply either, because the URL does not have a visible impact: a dummy URL would do just fine for preview purposes!
Although, come to think of it: a "call to action" component would want to have a required link, without wanting that link to ever be saved? So, perhaps there is a difference in preview impact (image/video URLs vs link URLs), but the saving impact is the same: nobody wants links pointing to https://example.com/campaign
, right? :D
Given all that, I think that UrlPreviewPropSource
would be a better name, that is much narrower and hence better captures both the need and when it is appropriate.
Thanks for raising this β and looks very cool!
How do you imagine this would work? Which SDC props should accept this?
- Is it an existing shape (
type
+format
β¦)? - Is it a new one? The icon equivalent of
$ref: json-schema-definitions://experience_builder.module/image
?
#53 π±ππ€£ gitnoob
And you got it character-by-character identical except for one line :O
#55: Not opposed to that, but it does mean that the literal originally reported issue does not get fixed. How about we just do 2 MRs for this single issue? And just merge this first MR β¦ first? Avoids issue queue overhead.
The issue title has been lacking precision since the start, which adds to the confusion. I'll think of a better title with a fresh head in the morning.
Thanks for confirming.
Re-testing.
β¨ Auto-save code components Active is in, and responded on the MR π
β¨ Auto-save code components Active is in. π
Follow-up for π Media Library dialog styling Active .
See review on MR; AFAICT this is blocked on β¨ Auto-save code components Active . Notified @tedbow over at #3500042-37: Auto-save code components β an hour ago.
Keeping issue summary relevant. The current MR should do only one thing, but there's a second part to thisβ¦
@larowlan is out this week ποΈ
We're threading very closely to this over in the MR for π Adding the Image component results in a state considered invalid Active .
Thanks, I was similarly confused.
@lauriii: Are you saying it's okay for the page title to not appear anywhere? Because if so, that's a trivial change π
wim leers β made their first commit to this issueβs fork.
- π± Excellent sleuthing! π
- Yep, I've noticed there's plenty of missing OpenAPI validation. But looking at it more closely again: why is that not triggering
\League\OpenAPIValidation\PSR7\Exception\NoPath
?! π± - π
This makes it all the more important to have proactive OpenAPI maintainers on this project, because it's supposed to save us time, not cost us time. And if the infrastructure doesn't let us do
It's feeling more and more that OpenAPI is less and less worth it?! π Feels like we need to spend some dedicated time to make it actually robust.