This needs to be fixed in src/features/code-editor/component-data/Props.tsx, and tested in tests/unit/code-editor-component-data-props.cy.jsx.
#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. 🙂
balintbrews → created an issue.
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.
Let's postpone this on #3552788: Automate the release process → .
We discussed this with @justafish and agreed:
- Committing the release artifacts should happen as a detached commit. The release tag Drupal.org will pick up will get added to this commit.
- We'll need to add
npm version $TAG --allow-same-version --no-git-tag-versionfrom the original script. - 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.
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.
The initial implementation landed in ✨ Extension plugins, UI, and browser API for reading component tree Active .
I tested manually to make sure the test failures are not showing a real problem.
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.
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.
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 .
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.
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 .
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.
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.
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 .
@jessebaker pointed out this issue: 🐛 Browser's undo state causes weird things with our undo history Active .
+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.
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.