🇳🇱Netherlands @balintbrews

I realized another consequence of this issue in relation to working with Code Components outside of the in-browser code editor, using the CLI tool. I updated the issue summary to reflect the current reality.

balintbrews → created an issue.

The plan I outlined in the ADR was already approved by @lauriii. He and I worked together on the metadata schema, so that's also validated from a product standpoint.

balintbrews → changed the visibility of the branch 3514033-adr--extensions-api to active.

balintbrews → changed the visibility of the branch 3514033-adr--extensions-api to hidden.

balintbrews → changed the visibility of the branch 3514033-adr--extensions-api to hidden.

balintbrews → changed the visibility of the branch 3514033-adr--extensions-api to hidden.

balintbrews → changed the visibility of the branch 3514033-adr--extensions-api to hidden.

This happened in 📌 Move code component starter template documentation from inline comments to online docs Active .

.

balintbrews → created an issue.

It seems that this is NOT introduced by ✨ Include navigation events in undo/redo Active . I was able to reproduce with a version earlier than that, and with React.StrictMode disabled. The changes done in ✨ Include navigation events in undo/redo Active also make it visible in React.StrictMode.

So this might be tricky. When we add a component to the editor frame, the instance receives a unique ID, right? If we undo that, then redo, we'll end up with a new UUID. So trying to get back to the previously assigned UUID in the URL wouldn't make sense, because that ID is not valid anymore.

#2: Whoever is working on the fix, they will need to look into this anyway, so I see no reason for postponing with that status. We may want to postpone this, however, for different reasons. 🙂

I think we should get 🐛 Redo action does not update the URL Active right first, and then tackle this. There may not be anything to tackle at that point. Though it's interesting why this happens only with that one component based on Mayur's testing.

balintbrews → created an issue.

Playwright test failure on this MR matches an earlier failure from one of the recent commits in 1.x.

Ready for review with approach #2 implemented in merge request !141. Heavily unit-tested, including the hook that syncs the route information between the Redux store and when navigating with React Router. I'll add a simple end-to-end test in ✨ Include code editor in undo/redo Postponed .

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

While we're here, can we please make this change? It would be the last thing to make the library look polished.

diff --git a/src/Plugin/Canvas/ComponentSource/JsComponent.php b/src/Plugin/Canvas/ComponentSource/JsComponent.php
index 75fc6432..63cf5ee2 100644
--- a/src/Plugin/Canvas/ComponentSource/JsComponent.php
+++ b/src/Plugin/Canvas/ComponentSource/JsComponent.php
@@ -291,7 +291,7 @@ final class JsComponent extends GeneratedFieldExplicitInputUxComponentSourceBase
       'id' => self::SOURCE_PLUGIN_ID . '.' . $js_component->id(),
       'label' => $js_component->label(),
       // @todo Update in https://www.drupal.org/project/canvas/issues/3541364. This causes a "@todo" Folder to be created.
-      'category' => '@todo',
+      'category' => '',
       'provider' => NULL,
       'source' => self::SOURCE_PLUGIN_ID,
       'source_local_id' => $js_component->id(),

I realize it's not related to this issue, but it helps reducing the clutter we don't need anymore, and we have a @todo comment anyways to do this properly.

📌 Hide "Add new folder" button under a feature flag, move "Add new code component" button to Manage library Active took care of this.

@omkar-pd, thanks for your previous work on this, and sorry that we missed reviewing and moving it along. I credited you on the other issue.

Yet another massive @jessabaker-style refactor and UX improvement! 👏 Thank you for going above and beyond.

I quickly added the debounce asked by @hooroomoo. While testing, I discovered 🐛 Hovering library items sometimes opens preview thumbnail with wrong size Active , opened as a follow-up so we don't block the huge changeset and all the UX improvements this MR brings.

balintbrews → created an issue.

Works beautifully! And the fact that you took care of the CLI tool, and made things work there, too, is very impressive. 👏 Thank you for another great contribution!

Sorry, I had the issue open from earlier when I updated the status.

#5: PR merged, included in tailwindcss-in-browser 0.4.0. We're now able to update src/features/code-editor/hooks/useSourceCode.ts to make use of the new compilePartialCss()function before transforming the CSS.

I'm in favor of this change, so are @lauriii and @kristen pol.

With that said, we discussed this with the broader team, and while there is agreement that revisiting our naming scheme is warranted, we would like to postpone this work and discussion, so we're not adding more to our plate before tagging the beta, which is finally at arm's length. The idea of ✨ Add a generic JavaScriptComponentSource Active is definitely exciting, and we would like to revisit this discussion once we have more breathing room to tackle that.

balintbrews → created an issue.

I discussed this with @minnur, and I see why this is a problem. Even if someone downloads a tagged release, if their workflow involves committing the entire codebase to version control, our .gitignore rule will be problematic. There's git add -f, but it's almost guaranteed that people would need to troubleshoot why the dist folder is missing before they realize they need to force add it. Many hosting providers operate with Git repositories, so this will be common.

I was thinking we could extend our tag-release.sh script to remove dist from the .gitignore file before the commit for the release is made, then add it back in the next commit. Permanently removing would cause friction for developers, so I wouldn't recommend doing that.

Follow-up created: 📌 Extend test coverage for code component data provider logic Active .

balintbrews → created an issue.

@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.