🇳🇱Netherlands @balintbrews

balintbrews → created an issue.

I updated the issue summary to reflect the suggested approach in #17, which will also help with avoiding errors that we've been seeing when someone tries to edit code before the WASM files are ready.

@pameeela: Are they broken broken? In 📌 Update image rendering to use the Canvas responsive image Active I made sure they're not by adding the intrinsic sizes to all examples, which made the images appear, but not with an optimized srcset, just with a regular src attribute. (I think if we could solve this issue, that could potentially make the sizes not required.)

We still would like to do this, but using a different approach, which I outlined in ✨ Upload third-party dependencies for Code Components using the CLI tool Active .

balintbrews → created an issue.

balintbrews → created an issue.

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

Thanks for confirming!

I read the relevant part of the spec at WHATWG HTML Living Standard, and confirmed two things:

1. With our current incorrect value 100vw will be used. (Changing priority of the issue to normal because of that.)
2. We can simply change it to "auto, 100vw", no need to do extra logic, browsers will do that for us to figure out if they can use auto, or if they should default to 100vw.

I updated the issue summary to reflect this.

balintbrews → created an issue.

Reviewed and tested this in Canvas, the changes worked great! 👍

I updated all examples to provide the width and height attributes. Without those the rendering of those components were failing. I also added explicit sizes attributes wherever it made sense. I tested thoroughly in Canvas — not like previously when I approved in #14 🤦 —, but a final review would be nice.

I also opened these two Canvas issues:

1. 🐛 Incorrect default sizes attribute for optimized images Active
2. 🐛 Image SDC does not generate srcset attribute for example images Active

Note that the second means that you need to select an image from your media library to test getting the optimized scrset attribute.

balintbrews → created an issue.

balintbrews → created an issue. See original summary → .

Oops.

balintbrews → created an issue.

balintbrews → created an issue.

I discussed this with @effulgentsia, and he pointed out that it's already possible to add the #import_maps attribute to a render array, e.g., in hook_page_attachments, and those will be collected. We should verify it, and also how that allows replacing an existing item.

Even if that already fully work, #1 from the issue summary is still a necessary piece.

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

Reverted.

I reviewed and approved the changes after fixing a small issue. Assigning to Pam for final sign-off.

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

All looks good, although I didn't have the time to test. I think we also wanted to make the overlay_opacity prop of hero-billboard required, and set '0%' as the first example value, so it's what gets selected by default in Canvas.

Very nicely done, and the new pattern with the single maps basically sets us up for html_cva if and when we're ready to adopt it. The diffs will be minimal.

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

balintbrews → created an issue.

Many Cypress specs failed in the last pipeline after I merged the latest code from 1.x. I ran them all locally, they all passed, and since we have had issues today with GitLab, I proceeded to merge the MR.

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

The new page: https://project.pages.drupalcode.org/canvas/code-components/cli-tool/pro....

Preview of the new page: https://project.pages.drupalcode.org/canvas/mr-315/code-components/cli-t....

balintbrews → created an issue.

We don't. I've been meaning to ask input from @bnjmnm to see if that even makes sense.

Example pipeline for a commit violating Prettier formatting rules: https://git.drupalcode.org/issue/mercury-3556548/-/pipelines/656981.

Turns out ESLint and Stylelint have never been set up for the project. Since both exclude any stylistic rules anyway in favor of Prettier, I just went ahead and added scripts to package.json to work with Prettier, and added the format check as part of the newly added lint job.

I then reformatted all files. Many of them were producing parsing errors due to techniques that static analysis tools can't work with. Fixing these also result in better readability.

Here are the types of errors I had to fix manually (marked with "⚠️ Refactored" comment in GitLab):

1. Even though this is how it's done in Drupal core's template files, we can't append {{ attributes }} to HTML tags without a space. For example, instead of <div{{ attributes }}>, we need to have <div {{ attributes }}>. I won't highlight all the places where I fixed this, because this is a trivial change.
2. Two files, components/button/button.twig and components/blockquote/blockquote.twig added attributes with inline Twig control structures ({% if %}), and/or had them directly within HTML tag attributes. I refactored those to assign values to variables.
3. Many files had opening and closing tags split conditionally. For example:
   

   {% if url is not empty %}
     <a href="{{ url }}">    ← Opening tag in one conditional
   {% endif %}
     <p>...</p>              ← Content outside
   {% if url %}
     </a>                    ← Closing tag in another conditional  
   {% endif %}
   

   This will never be static analysis friendly. But a better reason is that it's also not very readable. A similar issue is using dynamic tag names, like <h{{ heading_level }}>. I refactored the following files using Cursor, but I haven't had the chance to do manual testing:

   • components/card-profile/card-profile.twig
   • components/collapsible-section/collapsible-section.twig
   • components/heading/heading.twig
   • components/menu-footer/menu-footer.twig
   • components/menu-footer/menu-footer~main.twig
   • components/menu-utility/menu-utility.twig
   • components/menu-utility/menu-utility~main.twig
   • components/pager/pager.twig
   • templates/views/views-view.html.twig

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

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

So the grouping is still something we do ourselves, but if done this way, prettier will not undo it.

Right, the grouping should be subject to opinion. Sometimes you don't even want it, other times you would want to group based on the needs of the current component.

I think CVA sounds like a great option, but the refactoring probably is out of scope for 1.0.

👍

Just one other question, should we commit the other prettier fixes all at once? There are changes to almost every Twig file when I run it locallly.

Yes, but we should also make sure that a Prettier check runs on CI, otherwise it's easy to commit without formatting. We would need scripts in package.json, e.g.:

{
  "scripts": {
    "format": "prettier --write .",
    "format:check": "prettier --check .",
  },
}

Then we may want to add a .prettierignore file to exclude certain files. I was going to add all these, but I got confused, so I stopped to ask some questions.

1. There are scripts in package.json to lint the JavaScript code with ESLint, and the CSS code with Stylelint. But I don't see config files for these. I spotted eslintConfig in package.json, but it's not being picked up when I run pnpm lint:js, and it fails with an error message telling us that it is missing the config file. Stylelint does seem to run, maybe it's running with the default Stylelint config. So I'm curious how these are actually used.
2. There is no CI job for running the linting. I was going to just add a new one for the Prettier check, but since there is no job for running the linting currently, I thought I'd ask why that is.

We need to get these tighten up, because adding formatting with Prettier while not doing any linting and format checks on CI will be very disruptive. Imagine I want to submit an MR, and I want to format and lint my code, but I also get other files changed, because those may have been committed without any quality check.

I realized that the way we added grouping in 📌 Group Tailwind classes in Twig for readability Needs review doesn't communicate to prettier-plugin-tailwindcss where it should look for class names to sort: Prettier runs static analysis only, so it can't figure out that what we define in a variable will end up in the class attribute. I also missed that the class names that we would like to group together should be added as a single string rather than separate strings in an array. So what we're currently seeing is just @zackad/prettier-plugin-twig formatting the array, and prettier-plugin-tailwindcss not recognizing that it needs to do something.

→ The fix is to move the array inside the class attribute, and keep class name groups in single strings: da5afd69.

There would be a much nicer way to do this, and that is making use of the html_cva function, and setting "tailwindFunctions": ["html_cva"] in prettierrc.json. Two for the price of one, we'd get sorting, because that would tell prettier-plugin-tailwindcss to sort inside the arguments of that function call, and html_cva would give us a really elegant way to handle variations in the same component, so we could abandon conditionally concatenating the class names. (We already bundle CVA for Code Components in Canvas to encourage this pattern.)

That all sounds awesome, but there are complications:

1. We'd need to find/write a Drupal module that exposes html_cva from twig-extra-bundle, and make Mercury dependent on that module.
2. Mercury's Storybook setup doesn't use Drupal for rendering (which could be done with the Storybook module → ), so that environment wouldn't know about html_cva. I'm not sure what it would take to make Twing aware of html_cva, and inject the implementation of the CVA JavaScript library for the rendering. Quickly scanning Twing's documentation, it doesn't seem like it's easily pluggable. So I think with this we're hitting one of the limitations of not using Drupal for rendering the stories.

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

#18: That was added as a quick test when that method was introduced. The legacy extensions API is now deprecated, so it's not needed, but I just proceeded without removing it, because let's tackle that in a refactor.

I think Canvas developers should buy drinks for everyone who has been putting up with all the rough edges and providing invaluable feedback. 💙

Also, #11: Boooo! (Just kidding. 🙂)

I think this is a great idea! The Tailwind Plus templates made by the Tailwind team itself do the same: The different lines are always logical groups that make sense in the context of the styling being done. To automatically sort the class names within the lines (see the recommended sorting), we could use the Prettier Plugin for Twig along with the official Prettier plugin for Tailwind CSS.

balintbrews → created an issue.

We've made UI changes since this was reported.

I haven't seen this issue for a long time. @lauriii — have you seen it by any chance?

✨ Include navigation events in undo/redo Active made the problem less severe.

Does this mean we can/should remove updateExistingComponentValues? We also have _updateExistingComponentValuesForLinking, which is doing similar workarounds. Should we account for these in this issue, or would that be more appropriate to tackle later?

I don't see updateExistingComponentValues being used anywhere in the codebase. I know it was used before by Canvas AI UI code, but looks like it's gone. _updateExistingComponentValuesForLinking is being used by the content templates functionality, so maybe we should wait for 📌 Add e2e test for `ContentTemplates` feature Active to land, then attempt to refactor?

As we're picking this up again, I would like to explore doing the preloading explicitly in the UI app, so we can avoid having to make backend changes when we update tailwindcss-in-browser in the UI codebase, which could bring in the Lightning CSS WASM file with a new version number appended to the filename. (I understand there is a kernel test added to verify that, but since this is an update in the UI package, I would prefer if we dealt with it and verified it in the UI codebase. Otherwise we'll also need to make sure to run kernel tests on CI when the package-lock.json file changes.)

The default export from both lightningcss-wasm and @swc/wasm-web is a function to initialize the WASM file. We can call those functions in the UI app high in the component tree, and even set a state to present different UI elements before they're ready when the code editor is opened, and most importantly, prevent running compilation for a code component before the WASM files are available. We wouldn't have a way to know when that is without using the init calls.

Here is how to test this change.

To apply the change locally, run npm install and npm run build in the ui folder, then make sure to do a hard refresh in your browser.

Create the following Code Component:

export default function() {
  return (
    <p className="hidden md:block">
      I should be visible on medium-sized screens.
    </p>
  );
};

1. Test in an environment where the change by this issue is not applied. Notice that the paragraph is never visible.
2. With the change by this issue, the paragraph needs to become visible on medium-sized screens.

balintbrews → created an issue.

These issues occurred in a certain environment with Canvas.

Background color class is being overridden by another CSS rule

This was caused by a theme including the core/normalize library. Removing that solved the issue. I'm thinking about how we may want to detect from Canvas if that library is used, and suggest actions for the user, but I'm not ready for a proposal and a well-written issue for this.

Tailwind classes like hidden md:block aren't working as expected once the component is placed on the screen.

This was caused by hidden.module.css from the core System module. Doing a library override to move the .hidden class into a layer helped. Here is a new idea how to solve that. I'll create the d.o. issue for bringing in the new tailwindcss-in-browser version if we decide to go ahead with the solution.

Dynamic components like Main Navigation (component), Navigation Menu (Demo component) aren't rendering as expected on a page

Again, specific to that environment, I described the problem and the solution in this issue for a longer term fix: 📌 Evaluate support for preact/hooks specifier in Code Component import statemements Active .

balintbrews → created an issue.

After digging into this, and also verifying it with a project using regular Tailwind CSS via Vite, I realized that this is the expected behavior. It was not the behavior in Canvas previously due to us missing the CSS layers in our output, which was fixed in ✨ Modifying Tailwind theme in global CSS Active a few weeks ago.

The solution is to move those declarations in the global CSS inside the base layer. For example:

@layer base {
  a {
    font-weight: 700;
  }
  p {
    font-weight: 400;
  }
}

See the Tailwind CSS docs about adding base styles.

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