Amsterdam, NL
Account created on 4 October 2009, about 16 years ago
#

Recent comments

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

#6: I opened πŸ“Œ Audit Code Component HTTP endpoints to allow omitting properties Active for that. Let's merge what we have here, since it's already a solution with tests, and the patch were successfully aiding all our DrupalCon Vienna demos last week. πŸ™‚

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

Linking to the related core issue for visibility. In the meantime, since we're already doing our own import map anyway, we should allow this alteration. Once there is a core API, we can revisit this.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

We discussed this with @justafish and agreed:

  1. Committing the release artifacts should happen as a detached commit. The release tag Drupal.org will pick up will get added to this commit.
  2. We'll need to add npm version $TAG --allow-same-version --no-git-tag-version from the original script.
  3. If it makes sense here, we could address πŸ› Unable to add Drupal Canvas UI to GIT repository due to .gitignore rule Active , but it's also fine to tackle that after this issue lands.
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

Here is what's happening. Our logic that calculates data dependencies based on a Code Component's JavaScript source code incorrectly assumes that if anything is imported from @/lib/drupal-utils, we'll have a data dependency for something in drupalSettings, and initiates it as an empty array. For getPageData() and getSiteData() from @/lib/drupal-utils this is correct, and that array will be filled with the correct dependencies. However, @/lib/drupal-utils also provides sortMenu(), which is purely a utility function with no data dependencies, so the array remains empty if nothing else is imported that would warrant a data dependency.

So this import in the attached Code Component will produce the empty array:

import { sortMenu } from '@/lib/drupal-utils';

In my opinion, the backend is way too rigid to error out because of that, but that's a different discussion, and is a broader one than the scope of this issue. So I'm solving this from the frontend codebase, where the easiest thing to do without breaking changes is to make sure that we never send an empty drupalSettings array in dataDependencies.

Workaround until the fix lands:

import { sortMenu, getSiteData } from '@/lib/drupal-utils';

Just importing that additional package will add something in dataDependencies.drupalSettings, and will avoid the error.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

The initial implementation landed in ✨ Extension plugins, UI, and browser API for reading component tree Active .

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

I tested manually to make sure the test failures are not showing a real problem.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

The way to validate this would be running ESLint, and using the no-custom-classname rule from eslint-plugin-tailwindcss. So it's not easy to add until we figure out a way how to make it happen that we can run ESLint and other JavaScript-based code to check the output.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ changed the visibility of the branch 3514033-adr--extensions-api to active.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ changed the visibility of the branch 3514033-adr--extensions-api to hidden.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ changed the visibility of the branch 3514033-adr--extensions-api to hidden.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ changed the visibility of the branch 3514033-adr--extensions-api to hidden.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ changed the visibility of the branch 3514033-adr--extensions-api to hidden.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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 .

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

πŸ“Œ 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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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!

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL
πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

balintbrews β†’ created an issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

Oh, great, so we can close this issue.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

πŸ‡³πŸ‡±Netherlands balintbrews Amsterdam, NL

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.

Production build 0.71.5 2024