Opening an MR. I kept the cmdk code so it can be tried out in Storybook.
Here's some screenshots from implementation w/o cmdk
cmdk also doesn't support object values when selected, only strings. And it messes with them per https://github.com/shadcn-ui/ui/issues/889
I know it's been widely adopted, but as I keep researching I keep finding wild workarounds for it for folks using it because of shadcn/ui.
I quick asked @bostonjillian on how we should handle long page titles and slugs:
We could make this a bit wider, and let's do a character count cap with ellipses for like sentence level titles. Anything more than like 5 words is too long. Hover state, do a tool tip with the full title when hovering over the title.
After rechecking the designs, the navigation shows the page title and its path. Can we have the API also include the path for the page.
I'm working on using cmdk. I'm not 100% sure if it's the best approach since its filtering is based on available items. I'm not sure how well it works searching against a large dataset. https://github.com/shadcn-ui/ui/issues/868 has a related example/problem for combobox that is powered by cmdk. The main benefit is that cmdk has all the accessibility and keyboard interactions.
But the design shows a kebab menu for additional actions, which isn't a normal part of cmdk. I'll push forward a bit more. I don't think we'll lose a lot if it's not the way to go.
required title cannot yet be specified by the author.
The title is populated as "Untitled page"
It is also specified that it should be unpublished.
This was approved by laurii and I believe effulgentsia
Edit:
The UX is: Click "New", brought to editor for a page named "Untitled page"
Started messing with this and I want to sync w/ balintbrews about using the cmdk library versus Radix primitives directly.
Adding in https://www.drupal.org/files/issues/2025-01-16/selected%20home.jpg → as well
Going to take a stab at the failure POST 403 /web/xb-field-form/node/1?.
After looking at the update route
experience_builder.api.content.update:
path: '/xb/api/content-update/{entity_type}/{entity}'
I suppose it should be
experience_builder.api.content.create:
path: '/xb/api/content-update/{entity_type}
But we have logic in the controller which fails if the entity type is anything else but xb_page (for now.)
After reading 🐛 Controllers performing data modification should make use of CSRF tokens via /session/token Active , I think the approach should be changed. Originally the idea was just a regular link which would redirect to the editor. Instead it should be a POST request and then redirect based off of the response data.
Request:
POST /api/create/xb_page
Response
201 Created
{
"data": {
"xb_page": {
"id": 123
}
}
}
Then the editor can redirect to /xb/xb_page/123.
This unblocks the need for CSRF since we're using a POST method
This can be worked via storybook. Assigning to myself to track down design screenshots and previous notes.
Technically this is also blocked on ✨ Show page information in top bar Active
amangrover90 on my team is picking this up, assigning to myself since I can't assign to him
I don't know if we should really be using this ticket as there isn't pre-planning happening on D.o, but I can keep associating tickets to this. If you would like to, we should chat on the best way to accomplish it.
phpstan-drupal uses a stub provider, so we can fix here and back port anything to phpstan-drupal and use a version check on Drupal to decide if the phpstan-drupal stub should be loaded or not
lostcarpark → credited mglaman → .
Next steps for this meta will involve around the in-XB navigation. Working on a doc and issue map. I'll update here once there are more next steps.
Personally I think we should leave the restriction in for now and open a follow-up to add test coverage.
Agreed. The main goal was to ensure we have two testable paths, as there is some upcoming work which will be made a lot easier by targeting pages only.
now f.mazeikis can review :) https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... should fix it
Ugh, I looked too fast:
self::assertArrayHasKey('xb_test_page', $this->drupalSettings);
This is from the test module hook xb_test_page_xb_page_view:
function xb_test_page_xb_page_view(array &$build): void {
$build['#attached']['drupalSettings']['xb_test_page'] = [
'foo' => 'Bar',
];
It's to verify \Drupal\experience_builder\Entity\PageViewBuilder::alterBuild. The hook needs to specify the library I guess.
Merged, I feel the power! 💪
Perfect, thanks for optimizing
I'm slightly confused since the XB controller specifies drupalSettings in its render array. I don't get why removing the library definitions would break the test...
Unless in Kernel test the automatic addition of the library doesn't happen like in a fully bootstrapped system? I'll dig in.
The code was:
$page = $this->entityTypeManager->getStorage('xb_page')->create([
'title' => $title,
'uuid' => $uuid,
'path' => [['alias' => $path]],
'status' => TRUE,
]);
$page->save();
Which is what a lot of developers may do, since most do not call `validate` before saving. I have a feeling people will programmatically create the entities and while technically invalid expect them to work without XB crashing.
I'll keep assigned to me. amangrover90 from my team is picking this up and I pinged him to make sure he reviews. I like the approach, good idea @wim leers!
+1 @dpacassi your MR worked perfectly. Closing mine for https://git.drupalcode.org/project/gin/-/merge_requests/510
@dpacassi sorry for the delay, but testing the patch now with rc14. Just rebased the MR
Copying in the full error so people find this issue more easily.
So its override may lead us to lost some features.
That is fair. Right now the Gin theme is completely broken if the module is turned on. What if we leveraged ignore missing to allow progressive enhancement once the toolbar-button.html.twig is added back?
From the docs: https://twig.symfony.com/doc/3.x/tags/include.html
You can mark an include with ignore missing in which case Twig will ignore the statement if the template to be included does not exist. It has to be placed just after the template name. Here some valid examples:
{% include 'sidebar.html' ignore missing only %}
I just started, not on the main team though. I'll check that issue tomorrow and raise it internally.
🫠 apparently I search all the wrong ways. Sorry, I wanted to avoid making a duplicate.
I'm working on XB and using Gin. We're going to need the top bar, so we'll patch around it.
Not sure how heavy to go into the tests.
mglaman → created an issue.
I know one thing. Find all items like assert($entity->hasField('field_xb_demo')); to support pages and their components field.
Assigning to @laurii to help flush this out. The remaining item is 📌 Page should have author/user reference Postponed: needs info but I'm not entirely sure what is next. Maybe pivoting from `node/article` to `xb_page` and pushing forward other features in XB. I'm not sure.
Replied about the enhancer. And the test passed once I added video recording to debug... so let's see if it passes again when I remove videos.
I have to completely rewrite ApiPreviewControllerTest due to its usage of XBTestSetup. Unless we skip defining dependencies in the test base class, which is completely useless. Perhaps it should have all gone into a trait as I was wondering in my MR.
Because you cannot add dependencies to be installed on the base test. The test this issue added runs the site setup install script, which manually installs modules. It assumes a non-kernel test environment.
Thanks
Per #3452712-16: Possibly script loading/placement issue → and the reference core issue, this should fix some problems. We need the main library in the header. But AJAX should be okay without that toggle.
Here's where/why it was added: 🐛 Possibly script loading/placement issue Fixed
There's a reason we needed it to be in the header. Unfortunately I can't recall why. I'll look.
This copied \Drupal\Tests\experience_builder\Kernel\ExperienceBuilderTestBase and broke any test using it when dependencies are installed due to how \Drupal\Tests\experience_builder\TestSite\XBTestSetup operates.
Needs review. I think we can update some @todo to point to
📌
Consider removing baseQuery in favour of explicity requiring the params in each api call
Active
.
#20 still requires the field module to be installed, even though generic field support has been added:
// Call proxy implementations.
if (\Drupal::moduleHandler()->moduleExists('field')) {
_field_token_info_alter($info);
}
The module exists check can be removed.
And then I saw
$provider = '';
if (isset($info['types'][$token_type]['module'])) {
$provider = $info['types'][$token_type]['module'];
}
if (!($field instanceof FieldStorageConfigInterface) && $provider != 'token') {
continue;
}
Adding as feedback to that issue.
mglaman → changed the visibility of the branch 3479454-php-8.3-deprecations to active.
Applied suggestions, thanks @wim leers. I want to clarify how in depth we should provide a suggested solution for point 3 on.
The MR has been updated to use the approach found in the json_field module, where SQLite uses the "text" type, avoiding the core patch.
Okay, so what's left? Add this line to each driver? I don't know what value the in-code comment would provide. Or would the comment be for why we only added it to SQLite and not the others?
// @note: Only the SQLite driver has this field map to due to a fatal error caused by this driver's schema
// @todo: Add to all drivers in XYZ
'json:normal' => 'JSON',
Something like this?
re #15
1. tests can be run with mysql locally, and it's one test that fails on sqlite without the patch
3. blame Drupal core for not providing enough ways to govern things via code and magically through config objects 😬, I'll try to find a way though.
@wim leers metatag uses text for all? https://git.drupalcode.org/project/metatag/-/blob/2.0.x/src/Plugin/Field.... I think I know what you're talking about, though. Is it the json_field? https://git.drupalcode.org/project/json_field/-/blob/8.x-1.x/src/Plugin/...
Note they use text for the sqlite_type! XB does not.
'columns' => [
'value' => [
'type' => 'json',
'pgsql_type' => 'json',
'mysql_type' => 'json',
'sqlite_type' => 'text',
'not null' => FALSE,
],
],
];
re #9 I highly doubt ✨ Add "json" as core data type to Schema and Database API Needs work is going to make enough progress anytime soon.
we should at least at testing for the change and a comment about why the change is done.
I'll see if I can make a quick enough test. Hopefully without a field type and just manipulating a table with a JSON column.
This has uncovered 🐛 Cannot delete a field which uses JSON type Active
Ready for some full reviews!
Should be set, now.
Found a flaw. The route experience_builder.experience_builder requires a saved entity. There isn't a "new entity" route yet. I don't know if this issue should be modifying \Drupal\experience_builder\Controller\ExperienceBuilderController::__invoke to allow $entity to be nullable.
'base' => \sprintf('xb/%s/%s', $entity->getEntityTypeId(), $entity->id()),
This would break if null. But the entity type is a route parameter, so it could be added as a method argument.
Then in the method, if the entity is null we could pass an ID of `0`?
The initial review was addressed
This is not done in that issue. This issue is to implement entity owner interface.
It also makes the standard way of doing "Who created the page" a lot easier (target default data table vs revisions)
The MR working on adding the entity type already extends Drupal\Core\Entity\EditorialContentEntityBase, so it'll be covered. But not the idea of "owning" a page like node and media already do
Technically the revision log gives us all of that information, revision log entries always reference a user.
This issue would enable "edit|delete own" and "edit|delete any" permissions.
mglaman → changed the visibility of the branch 3487533-cannot-delete-a to hidden.
ugh picked the wrong branch