Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Account created on 26 December 2006, over 17 years ago
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Both @jessebaker and I posted our reviews :)

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Note that I do not think this is in the critical path for 🌱 Milestone 0.1.0: Experience Builder Demo Active , but it would be awesome if we can get this in place before then for sure β€” we'll need it for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Postponed , because without it the data model would be very much incomplete.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I intend to review @larowlan's MR first thing on Monday :)

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Reviewing! 🀩

The MR is not running all tests currently, due to πŸ“Œ CI: do not run PHP-only jobs if an MR only changes the UI Active . I just merged a follow-up MR that fixes that problem: #3460778-6: CI: do not run PHP-only jobs if an MR only changes the UI β†’ . Merging in upstream to get all tests running in this MR πŸ˜…πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Looking good! Did my best to review this, but I think it'd be good for @jessebaker to sign off on this. (He's already awake, Ben is not yet.)

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ FieldType: Support storing component *trees* instead of *lists* Needs work is in β€” let's get this done next! First let's get this to green, and then I think merging #3460856 into this will be the least amount of effort, as described over at #3460856-5: [PP-1] Create validation constraint for ComponentTreeStructure β†’ .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I've read and re-read https://docs.gitlab.com/ee/ci/yaml/#rulesif and I can't make sense of why it's not working. Attempted fix on another issue/MR failed: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This caused a regression. CI jobs are not running when they should:

😭

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I have no idea how the pages CI job is getting triggered for you πŸ˜…πŸ€ͺ Never had that happen!

This is obviously actually passing all tests.

I just fixed a few nits, and I see the same status as the summary for the overall CI pipeline. That's just GitLab CI being silly/dumb: it's because the pages CI job is the last CI job (in the last stage), and hence that becomes the status/summary.

I think we can get rid of it by removing the manual rule β€” then it just won't show up for MRs β€” and that's indeed working: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Crucially:

  • Merge request pipelines, rules:changes compares the changes with the target MR branch.
  • Branch pipelines, rules:changes compares the changes with the previous commit on the branch.

β€” https://docs.gitlab.com/ee/ci/yaml/#ruleschanges

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I wrote #9 before I saw πŸ“Œ [PP-1] Create validation constraint for ComponentTreeStructure Postponed .

Why don't we merge#3460856 with πŸ“Œ Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work β€” i.e. why don't we expand the scope of that? It's exactly the same problem space!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I see this now, after I already wrote #3455728-9: FieldType: Support storing component *trees* instead of *lists* β†’ …

Actually … why don't we merge this with πŸ“Œ Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work β€” i.e. why don't we expand the scope of that? It's exactly the same problem space!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#20: hah!

I'm fine with doing additional things in follow-ups, but would like to see @bnjmnm's feedback in #18 addressed. That doesn't require Doing All The Things in this issue β€” an alternative can be to restrict what npm run format does. Could you do that, @finnsky? 🀞

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  1. πŸ₯³ It works! πŸ˜„
  2. // @todo This is a temporary checks until we have a constraint. Just to make
        //   tests fail if we don't have good test values.
        if (!isset($this->tree[ComponentTreeStructure::ROOT_UUID])) {
          throw new \UnexpectedValueException('Temp exception replace with constraint. Root UUID is missing.');
        }
    

    On a meeting just now I said I was fine with this, but … I now see that the constraint was part of this issue's scope, as mentioned in the issue summary.

    Having actually reviewed the MR, I am now convinced by that even more, because since ✨ Allow specifying default props values when opting an SDC in for XB Needs work landed, we now have strict config schema checking for all tests.

    Having explicit test coverage for this will also allow us to validate the default value in config/optional/field.field.node.article.field_xb_demo.yml β€” similar to what πŸ“Œ Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work is doing.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Actually, , because it needs input from @jessebaker, and this is a meta issue, so no actual implementation work happens here.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oops, sorry for overwriting your merging of upstream commits, @bnjmnm, that was unintentional πŸ˜…

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Remove example Cypress tests

Discussed this with @bnjmnm:

Pushed a commit for that just now!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Slightly expanding the scope, because I believe that is necessary for DX clarity. Especially after πŸ“Œ CI: improve UI-related CI jobs:`UI eslint` obscures TypeScript Compiler errors, `npm run build` runs too late Needs review and πŸ“Œ CI: do not run PHP-only jobs if an MR only changes the UI Active .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I've implemented this for the contrib module I'm currently working on: #3460778-3: CI: do not run PHP-only jobs if an MR only changes the UI β†’ .

Works well πŸ‘ Speeds up CI runs, reduces CI resource consumption, and improves signal-to-noise ratio of CI.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This MR touches only .gitlab-ci.yml, and so there's no point in running most CI jobs … which now actually is the case:

πŸ₯³πŸš€

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I see now the issue title was wrong β€” but the summary was right πŸ™ˆ

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

And now the Cypress CI job is faster! It no longer runs npm run build, not even npm install β€” because it receives all that work from the earlier UI CI job πŸ‘

Bonus: πŸ“Œ CI: do not run PHP-only jobs if an MR only changes the UI Active will improve the DX for JS UI-only changes further 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Wim Leers β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

A TypeScript error now results in NO more linting, NOR Cypress testing:

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Had to bypass CODEOWNERS because we're understaffed: @tedbow is out and @f.mazeikis is assigned to a different project this week. Went ahead and merged this β€” no shocking changes in here. Progress matters more than thorough reviews at this stage.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Found the solution to @lauriii's original bug report. πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

What is this postponed on? On design? On an outcome of the discussion at πŸ“Œ Clarify "components" vs "elements" vs "patterns" Active ?

This affects 🌱 [META] Configuration management: define needed config entity types Active and would be one of the logical next things to work on now that ✨ Allow specifying default props values when opting an SDC in for XB Needs work landed.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks, @larowlan! πŸ˜„

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#9:

Ah, I see how that's confusing!

✨ Allow specifying default props values when opting an SDC in for XB Needs work landed, and it did add support for a number of field types. But link is actually not one of them β€” the validation constraints only allow string, uri, file and image field types: https://git.drupalcode.org/project/experience_builder/-/blob/9ccf646d71c...

"link" was just intended to be an example … but as you demonstrated: a poorly chosen one! So, updated issue summaries to rectify that.

The link field type is out of scope here, that's what πŸ“Œ [later phase] Support props defaults that have config dependencies Postponed exists for.

This issue is intended only to make it possible to specify a default value for the file and image field types. ✨ Allow specifying default props values when opting an SDC in for XB Needs work did not add support for that: it only allowed selecting those field types/widgets, but no default value can be specified yet.

Tried to clarify that. (And spotted some very confusing language because of it πŸ™ˆ)

Cross-post! πŸ’₯

#10:

So to double check - what this does is to only base64 encode the image when configuration is exported, and then save it as a file entity when the configuration is imported - then on the site itself, the default value is still a file reference, not the actual base64 encoded image?

Yes, exactly!

That bit seems superficially OK but I'm not sure I get the full consequences yet - if I export that config again (to config/sync) then import it, is it going to keep getting converted between base64 and and a file reference (presumably with a new file over and over again) or is the file uuid also stored so that can be used if the file already exists?

Excellent question. Agreed we should not do that. But this seems easily solved: rather than generating a random UUID, generate a UUID from the base64-encoded representation of the file.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#3: I'm pretty confident @lauriii is referring to "fields with no visual representation" when he says "meta fields". Example: path alias.

An entity's title/label field is visible. Hence the title: "title and meta fields"

@lauriii: is this still assigned to you because you're workin on defining this more, i.e. are working with the Acquia UX team to get a design ready? IOW: is this blocked on design?

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@finnsky: Could you address #18? 😊 πŸ™ Would love to see this land!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Per #21.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oh, and @tedbow pointed out that there are related/similar problems in Layout Builder: πŸ› Imported layout builder settings fail if inline blocks are present due to missing revisions Active .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

For all other default values or config -> entity references in core (blocks is one example), we require that the entity exists on the site in order to be able to export the reference. This has some inherent limitations - like having to create content on production, sync it locally, then create the config, then sync the config

Correct, this is how it works by default, today.

Why uniquely diverge from that pattern here and start (I assume) base64 encoding images into config while disallowing other types?

Because @lauriii has clarified requirement 63. Shared components and

1.2 Design system (foundations)</a> as being able to share components from one site to another, including default values for props, using <em>nothing else besides standard configuration management</em>.

(That's a big reason for 
            
              
              
              🌱
              [META] Configuration management: define needed config entity types
                Active
              
             being so very important, and why there's such emphasis on config dependencies.)

That's why uniquely diverging for component config entities' default props was considered A) reasonable, B) required.

We labeled this the Big Default Content Question: instead of referencing content entities and hence depending on them, and hence needing recreate them/ensure they exist on another site, we base64-encode files/images (the 95% use case) to avoid that problem altogether. Additional entity fields/properties that are used such as alt, titles, labels could also be exported along with that.

See <a href="https://git.drupalcode.org/project/experience_builder/-/commit/8ecc46f9141b0e563605b33ebdae7baf1d7730fc"><code>tests/src/Kernel/ConfigContentExportTest.php

in @f.mazeikis' PoC for how this works quite elegantly.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
(e.g. the string, link etc. field types).

This isn't the case for link, it takes a URI, which includes the entity:// format, which has an implicit, if not explicit, dependency on the entity being linked.

Oh, that is a very good point! 😬

After I could not find any test coverage for this, I tried to create a Link field (allowing both internal & external links), and then entered entity:user/1 as the default field value … it refused to accept this?! 😳
So I continued without a default field value, and instead tried using it. Turns out this automatically uses an entity reference autocomplete for the same entity type + bundle (in my case: node:page). Huh?!

I then found test coverage over at \Drupal\Tests\link\Functional\LinkFieldTest::doTestURLValidation(). Following the trail of breadcrumbs, I ended up at \Drupal\link\Plugin\Field\FieldWidget\LinkWidget::getUriAsDisplayableString(), which contains:

    elseif ($scheme === 'entity') {
      [$entity_type, $entity_id] = explode('/', substr($uri, 7), 2);
      // Show the 'entity:' URI as the entity autocomplete would.
      // @todo Support entity types other than 'node'. Will be fixed in
      //   https://www.drupal.org/node/2423093.
      if ($entity_type == 'node' && $entity = \Drupal::entityTypeManager()->getStorage($entity_type)->load($entity_id)) {
        $displayable_string = EntityAutocomplete::getEntityLabels([$entity]);
      }
    }

… which was introduced by #2804391: Resaving menu links that points to a non-node entity changes the type to node and breaks the link β†’ in 2017. It narrowed the entity: linking functionality that πŸ“Œ Implement autocomplete UI for the link widget Fixed introduced in 2015, to only support node. So … until πŸ“Œ Allow multiple target entity types in the 'entity_autocomplete' Form API element Needs work lands, this is pretty much a non-issue, because the link field type only allows linking to entity:node/*, and hence only depends on the node module! πŸ€ͺ

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#6 was easy: I didn’t think to test adding new components. Fix available on that issue for this new bug you reported.

Still working to fix the first bug you reported.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Aha β€” this closes only the deepest level, the menu that lists and remains open. I thought #10 meant the entire thing should be closed? πŸ€”

Asking @lauriii to confirm.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Questions about lock file changes, the new SVG, and LTR/RTL considerations.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks, @fazilitehreem!

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

LGTM! Needs sign-off from @jessebaker or @bnjmnm, and @jessebaker is unavailable today, and this is so trivial, that I'm going ahead and merging myself.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

PHPStan & phpunit CI jobs are now passing.

I unfortunately can't locally see your awesome work in action, because I get this:

$ npm run build

> vite-template-redux@0.0.0 build
> tsc --noEmit && vite build

error TS6305: Output file '/Users/wim.leers/core/modules/contrib/experience_builder/ui/vite.config.d.ts' has not been built from source file '/Users/wim.leers/core/modules/contrib/experience_builder/ui/vite.config.ts'.
  The file is in the program because:
    Matched by default include pattern '**/*'


Found 1 error.

I wondered if something was wrong about my node setup (I'm still on version 18 πŸ˜…), but then I discovered that very same error appears on the Cypress CI job: https://git.drupalcode.org/project/experience_builder/-/jobs/2082413

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@larowlan wrote but did not click "approve" β€” so bypassed the explicit approval 🦹

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Ahhh, it's at /admin/structure/component/edit/experience_builder%2Bimage?destination=/admin/structure/component β€” deduced that thanks to the stack trace:

…
Drupal\experience_builder\Form\ComponentEditForm->form() (Line: 107)
…

Reproduced.

This is a bug in ✨ Allow specifying default props values when opting an SDC in for XB Needs work , which landed yesterday. I intentionally did not add test coverage and only tested manually, because we know it's all subject to heavy change. I must have accidentally broken it shortly prior to merging, because it definitely did work.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I cannot reproduce this using the exact same steps to reproduce, but using Drupal 10.3 instead.

You wrote:

When I try to edit the built-in image component, I get following TypeError:

Where are you trying to do that? At /xb (the PoC UI) or at /node/1/edit?

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ€” This is exactly what EndToEndDemoIntegrationTest is intended to prevent.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Add test coverage to A) reduce the concerns voiced by @larowlan, B) empower him/others to propose an alternative implementation. The test shows the range of needed capabilities β€” EndToEndDemoIntegrationTest already did that, but the new PropSourceTest does so in more detail, and in a kernel instead of functional test: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Assuming that the \u<unicode code point> aspect was a significant part of what @larowlan was referring to in his MR comment, that's easily solved by using https://www.php.net/manual/en/function.json-encode.php.

Note that \Drupal\experience_builder\PropSource\StaticPropSource::__toString() already uses that … it's just that https://git.drupalcode.org/project/experience_builder/-/blame/0.x/config... was handcrafted and does not yet have validation constraints that would detect/prevent this particular encoding. Fixed with a simple print json_encode(json_decode($s), JSON_UNESCAPED_UNICODE);: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... :)

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@larowlan Thanks! Merging in all upstream commits first…

Only developers would maybe see the string representation of (Static|Dynamic)PropSources. And they definitely won't be reading that representation in JSON-encoded-in-YAML form (although there we could do the work to not use \u<unicode code point> but just use the Unicode representation directly β€” we should do that, it's just not urgent, especially if we don't know we'll continue to use this approach).

This is a data structure ((Static|Dynamic|Adapted)PropSource are strongly typed PHP objects), it's just one that has an equivalent string representation. That string representation allows both for a more compact representation, resulting in less (cognitive & space) overhead when looking at an entire component tree. I wanted a string representation
because that makes searching (googling/ducking) for problems much simpler.

I respectfully disagree with you that only a data structure, without a string representation, would be better. You'd then at minimum still need an array representation to be able to express/store it in JSON/YAML, and I wanted to avoid a new Type Of Array of Doom πŸ˜… β€” but it sounds like you think this is perhaps the first Type of String of Doom? πŸ˜…

All that being said: this seemed the best/simplest path to something that works that I could see. If you feel so strongly that there is a better way, then I'm absolutely open to an alternative. Whether you provide that in the form of a photo of drawings on a napkin, sample JSON blobs or an MR, is your choice :)

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ Add some basic example SDCs Fixed landed β€” this MR can now be rebased and should become ~240 LoC smaller.

EDIT: I see that @bnjmnm already removed these πŸ‘

@bnjmnm I'll get the PHPStan things out of the way for you. 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This was a soft blocker to πŸ“Œ Add component edit form to contextual panel Needs work β€” allowing that MR to become smaller again. πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Done.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Add missing comma to proposed resolution.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks β€” I'll get those fixed!

This will also help make πŸ“Œ Add component edit form to contextual panel Needs work 's MR (https://git.drupalcode.org/project/experience_builder/-/merge_requests/53) smaller πŸ‘

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

⚠️ Note that this won't fix the problem demonstrated in the issue summary's GIF for the component, because for that πŸ“Œ [PP-1] Default props values should support files/images Postponed must first happen on the config management side.

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Finished manually testing the edit form that was added here. I got the tests passing by specifying the necessary config by hand, that was easier πŸ˜… Now I can confirm it actually works correctly, but I had to fix a bunch of things. I had not yet realized the ComponentEditForm was not yet running config validation, that'd have saved me a lot of time. But … now it is. So any future iterations will be far less painful πŸš€

Now this is truly ready to go β€” and @f.mazeikis has approved the MR in the mean time β€” I've essentially reviewed his work and built on top of it. His words: 😊

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I'm working to unblock @bnjmnm here by finishing ✨ Allow specifying default props values when opting an SDC in for XB Needs work and then landing πŸ“Œ [PP-1] HTTP API: update /xb-render-component/{component_id} to use Component config entity's default values Postponed β€” together, they will ensure that it becomes feasible to edit components without the horrors of TwoTerribleTextAreasWidget πŸ§Ÿβ€β™€οΈ

Crucially, that will allow @bnjmnm to fix the one inescapable bug he's been running into here: dragging in a new component onto the canvas results in fatal responses from the server to the client's request to preview an additional component (and then subsequently edit it).

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The issue that @bnjmnm is working on that I've been referring to: πŸ“Œ Add component edit form to contextual panel Needs work .

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Production build 0.69.0 2024