🇳🇱Netherlands @balintbrews

@jessebaker pointed out this issue: 🐛 Browser's undo state causes weird things with our undo history Active .

balintbrews → created an issue.

+1 from me to land this with a change we discussed on Slack: "only block deletion if [a code component] is in use in the default/active revision of >=1 content entity".

Once we have that, I think this is a great incremental change. @lauriii did express two things:

1. 📌 FE follow up to Only allow deleting Code Components if there's 0 usages Active needs to happen soon.
2. If we're not yet accounting for usages in auto-saved version of pages, which I don't think we are, that also needs to happen as a follow-up.

balintbrews → created an issue.

Oh, great, so we can close this issue.

@hooroomoo, you mentioned that you had already done this in an ongoing MR. Is that something that's landing soon? If not, can you please point us to what to cherry-pick?

In a Slack conversation between Wim and I, Wim suggested that we do this in code rather than config, and that "programmatic would be safer, unless you add a test that fails if the set of expected core block plugins change", then added: "[Tests are] necessary anyway. Because we want our decisions to cover all core block plugins always."

Given that this is a temporary solution, I don't think we need to put a lot of emphasis on the correctness, but I'll leave that to Wim to decide. One thing I would add is that we do want users to be able to enable these blocks (components representing block plugins) if they decide to do so.

balintbrews → created an issue.

balintbrews → created an issue.

Addressed both #8 and #9 in MR !29. I also made a CSS change earlier to verify that the job is now green when it runs.

The first three commits contain the relevant changes. The last one is reformatting the files.
I also wrote documentation on how I configured the plugin, which is part of the first commit.

balintbrews → created an issue.

wim leers → credited balintbrews → .

Re: using IndexedDB, I don't see why not. 🙂

What's in the MR feels manageable, but in case the code to interact with IndexedDB grows in the future, we can consider adding something like localForage as an abstraction.

balintbrews → made their first commit to this issue’s fork.

balintbrews → made their first commit to this issue’s fork.

Let's address the SA in a separate issue.

balintbrews → made their first commit to this issue’s fork.

jessebaker → credited balintbrews → .

I approved from the frontend side with the following observations:

1. Display both internal and exposed code components in Manage library under the Code tab. See #16.
2. When a new code component is created from Library, the Code tab of Manage library should be opened.
3. When a code component is clicked under Manage Library, and the current route is not the code editor, the Code tab should remain selected.
4. Appearance of empty folders need to be adjusted, see #note_575777.
5. Appearance of hovered folders need to be adjusted, see #note_575778.
6. Adding new folders should use @/components/Dialog and handle errors, see #note_575780.
7. Components and patterns should not be draggable to the canvas from Manage library, see #note_575781.

I think the issue can land without these. @bnjmnm, I'll leave it up to you how you would like to structure the follow-up issue(s).

A proposal by @jessebaker that we decided to implement:

In Manage library, let's display both internal and exposed code components under the Code tab. The backend already supports the organization into folders for JavaScriptComponent entities, which will be separate from organizing the Component entities, which will take effect under the Components tab — where exposed code components are represented by Component entities.

#14: I believe "break XB" means what was reported in 🐛 ValueError: "Drupal\Core\Template\Attribute" is not a valid backing value for enum Active , which was closed as a duplicate of ✨ How to pass content_attributes to SDC without it being shown in the XB UI Active . So that issue should be fixed now, except maybe what you pointed out, "the case of type being an array of types". Either way, separate issue.

Before @penyaskito's change landed, it was impossible to expose SDCs from the base theme, and there is a way to opt out, so it's definitely a great step forward. However, I'm still wondering how many themes out there follow the logic of Radix, where the sub-theme uses only a subset of the provided SDCs. If this is a common pattern, we might want to consider one of the followings:

1. Improving the opt-out experience with some focused tooling or UI to allow disabling all components from a sub-theme;
2. Changing the logic of loading SDCs from the subtheme to be opt-in.

balintbrews → made their first commit to this issue’s fork.

#12: That should be fixed as of ✨ How to pass content_attributes to SDC without it being shown in the XB UI Active .

Nevertheless, this might be something to consider. As far as I know the idea of the Radix theme is that it ships a lot of SDCs, and you cherry-pick the ones you would like into your sub-theme. Loading everything from the base theme would go around that. Maybe the idea is unique to Radix, or not that common?

On the other hand, you can still disable components for XB (/admin/appearance/component), so there is an option to exclude anything you may not want, it just requires an additional action.

I also ran into issues with setting a React state from a Deep Chat event handler, and without that the component rendering is not reactive. At one point we should look into how to refactor the component to make use of React state for storing the chat history. I'm sure it was tried when the implementation was put in place, and I'm sure it wasn't straightforward, so I want to acknowledge that it is probably that way for a reason. In any case, I'd be happy to discuss the challenges when the time is right.

I pushed a solution that uses Deep Chat's introMessage property in combination with the deep-chat-temporary-message CSS class. This is great because it's the logic we need provided by Deep Chat, but not so great because of the CSS trickery I had to do to make the message appear according to our design. Because of that, I'm totally fine if my commit gets reverted, and the previously working solution lands instead.

#8.3: Yes, we should. I discussed that with @lauriii.

Answers to questions in #5:

1. I missed when we added that. 🙂 Those need to be replaced by folders, which will be initially auto-created from those block categories in 📌 Expand Folder entity functionality Active . In other words, folders are superseding the hard-coded block categories.
2. Correct, single text field in a dialog, @/components/Dialog should be sufficient. The only requirement is that folder names need to be unique within a parent (i.e. components, patterns, code).
3. Confirmed. I'll ping @callumharrod for the visual example.
4. I would vote for grouping them together, starting with folders. @callumharrod, can you please confirm? It should be the same in Library and Manage library.
5. See ✨ Renaming, moving, and deleting folders through contextual menu Active .
6. @callumharrod, please.
7. We'll expand on that in ✨ Renaming, moving, and deleting folders through contextual menu Active , alphabetical order and 0 weights are what's in scope of this issue.

I just had a conversation with @lauriii. Internal code components should live under Manage library. I adjusted the issue summary accordingly.

Another thing just occurred to me. Yes, I'm sorry. I don't anticipate more changes, fwiw. 🙂

We need to support organizing pattern entities, too, not just components. So I recommend that we rename the components property to items.

We previously had code in the MR that ensured that folder names are unique within a UI library. Then I asked for removing it because the concept of UI libraries is going away. What I didn't consider was that we still need unique folder names for components in the library vs. internal code components (which will eventually move to a separate menu item). I'm really sorry, but we'll need to bring back some of that code. I think we shouldn't call the property uiLibrary as that concept will still go away. Maybe parent? Any other ideas?

I left a comment suggesting conditional rendering, but I'm pretty sure it was tried, so I'll take a look to see why that effect is not reactive to the history changes.

The issue summary already talks about nodes only, but I just wanted to reinforce that at this stage we're only targeting node bundles, no other entity types are in scope.

Edited the issue summary to note that only content types (node bundles) are in scope at this stage.

hooroomoo → credited balintbrews → .

Crediting Jesse for spotting a gap in the issue description.

Because of the data integrity issues, I wonder if we could prioritize the simplest possible change before we settle on the final solution, including soft-deletion etc.

I think it would be the following:

When users try to delete or make a component internal (i.e. remove from components), we currently do that check to see if the component is being used in the current page's component tree. We can now do that properly thanks to 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active . I don't see an HTTP API for the audit info, so unless I'm mistaken, that would need to happen before we update the check on the UI.

Do we already account for first-party code component imports (JavaScriptComponent dependencies) in the audit logic?

balintbrews → created an issue.

balintbrews → created an issue. See original summary → .

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

The discussion here became outdated. Closing in favor of 🌱 [META] Library organization with folders Active .

balintbrews → created an issue.

I added the uiLibrary property in the issue summary which will allow us to track which part of the XB UI the folder belongs to. Note that we also need to be able to have folders under Code, but Code is not something that \Drupal\experience_builder\Entity\Component::computeUiLibrary is aware of.

#9 — No, we can avoid that, see #3538775-4: Handle data dependencies in CLI commands → .

I was thinking about backwards compatibility with the 0.x branch, and I was ready to go ahead and start different versions for the CLI to address that, but @justafish had a better idea:

We could see if in our upload command we receive a 422 error complaining about the dataDependencies property not being recognized — which would happen with the 0.x branch. In that case, we could retry the request while removing dataDependencies from the payload.

I might have been wrong in #6, and we may need to backport anyway and start branching with the CLI tool versions.

Important note: While this is targeting the 1.x branch, due to 📌 Handle data dependencies in CLI commands Active , it needs to be tested with an XB module installation from the 0.x branch. Backporting code of the CLI tool has no practical benefit since it's a separate release process, but this is a bit inconvenient until we fix 📌 Handle data dependencies in CLI commands Active .

Adding credit for reporting the issue and investigating the problem.

balintbrews → created an issue.

I'm picking this up.

balintbrews → created an issue.

Adding credit for the original bug report.

balintbrews → made their first commit to this issue’s fork.

griffynh → credited balintbrews → .

Unassigning as I'm not working on this right now, in case anyone would pick it up.

balintbrews → created an issue.

balintbrews → created an issue.

The backport tag probably only applies to the first MR that's already in.