🇪🇸Spain @penyaskito

Potentially a duplicate of 🐛 UI/UX Canvas 'Add' and 'Manage library' tool tips and menu names inconsistent Active , unless we actually want to repurpose this to avoid the console error or even add a dialog when you drag from the library explaining the "Add" should be used.

See 📌 Only allow deleting Code Components if there's 0 usages Active

penyaskito → created an issue.

Apparently this isn't a bug but a UX issue.
This needs input from Lauri (and/or design?)

Added change record draft and deprecation error.

penyaskito → created an issue.

RTBC. Lee to confirm all threads can be resolved.

I'm RTBC-ing assuming the tests pass after my rebase (there was a conflict in ui/src/components/form/inputBehaviors.tsx but was trivial enough for me to fix it).

Some visual concerns raised above:

• The link widget help is a lot.
• The checkboxes looks like too much information so condensed.

But these can be definitely follow-ups needing design input.

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

Ted, Wim or Alex need to approve this, tomorrow is a holiday in US and Belgium, so the first to be back on Wednesday is Wim, assigning to him.

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

LGTM, don't think this needs tests.

Yes please. Tagging need tests.

When I was about to merge this, @phenaproxima highlighted the results from user testing (glad I didn't want to do that before RC3).

IMHO we should merge this without the menu_links_discovered_alter (the Content → CMS rename), the link to Drupal Canvas pages is definitely needed *somewhere*. How are they labelled can be a follow-up after we get more feedback from user testing.

Given Lauriii confirmed I'll test and post screenshots tomorrow (without navigation) and merge.

I guess this needs Lauri's feedback.

To be clear, AFAIK whatever we do here for Canvas doesn't really need to be set in stone for Drupal CMS, we can fix in drupal_cms_starter as we do with the "Create content type" menu link, or even drupal_cms_helper.

Need some feedback for validating the new approach.

config schema: \Drupal\Core\Config\ConfigBase::validateKeys

You beat me to comment here, but I tested this yesterday and I didn't see any description in the widgets at all.
So wondering how the test is passing? I explicitly tried to reproduce that scenario manually, and the description were nowhere to be found.

Thanks everyone!

Hoping @larowlan can take care of this.

Created 🐛 Respect `allowed_formats` settings in \Drupal\text\Plugin\Field\FieldType\TextItemBase::applyDefaultValue Active and 🐛 Respect `allowed_formats` settings in \Drupal\text\Plugin\Field\FieldType\TextItemBase::generateSampleValue Active on Core's queue per #20.
Updated change record at https://www.drupal.org/node/3556442 →

penyaskito → created an issue. See original summary → .

penyaskito → created an issue.

Appreciate the intention, but let's have the "stable blocker" tag as clean as possible.

🐛 Text formatted with CKEditor within Canvas gets double escaped when output Needs work landed so this shouldn't be postponed anymore.

Created follow-ups

• #3556488: Enable `Drupal.Commenting.FunctionComment.Missing` check and fix existing issues →
• #3556487: Enable `Drupal.Commenting.VariableComment.Missing` check and fix existing issues →
• #3556486: Enable `Drupal.Commenting.ClassComment.Missing` check and fix existing issues →

penyaskito → created an issue.

penyaskito → created an issue.

penyaskito → created an issue.

Wondering if should be a duplicated instead, as we will want to fix both in the same release.

+1 RTBC. Needs CODEOWNERS approval in MR to merge.

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

#27 would also be alleviated if 🐛 Validation error on optional properties. Active lands

Needs followup per https://git.drupalcode.org/project/canvas/-/merge_requests/260#note_619017

"Drupal translation server" is still used in HEAD: https://git.drupalcode.org/search?search=%22Drupal+translation+server%22...

#10 makes sense and described the root problem. The code and tests changes cover the issue and LGTM.

Test-only failure as reference: https://git.drupalcode.org/project/canvas/-/jobs/7128096#L532

After reading this #10 makes sense to me. And if this needs to be solved for Drupal CMS, I wonder if providing that computed field should be the responsibility of canvas or a different specific contrib module.

This might be a duplicate of 🐛 DefaultRelativeUrlPropSource does not sort properties keys for comparison RTBC , and not related to the engine being used.

I'm happy to "adopt"

• Content Translation
• Language

I was part of the D8MI team, spoke about it a different DrupalCamps and DrupalCons, and worked on a translations service provider that had a Drupal integration I maintained for 7+ years. Currently collaborating with the teams behind the Multilingual track for Drupal CMS + Drupal Canvas.

For "removal" criteria we should be really careful here. Not only because of political reasons, but technical reasons:

system.date config entity uses this country list as a constraint. So a site NOT implementing the hook could have a country code, and if a later version removes that country, the upgrade could fail.

So even if we find any reason to remove a country code, that should only happen in a major release, never in a minor or patch release, and would require a change record.

This is ready to go.

The only blocker is properly clarify why some updates run multiple times, and why the number of component versions is different between the sdc and the js examples.

Didn't manually test this, but code looks good.

Tests pass and fails as expected. Wondering if we want a test for actually placing the block, but that would be probably be testing the block system itself, so RTBC.

Thanks!

This prevents now serializing a Component, which kind of covers #10.

If we ever need to do it, #10 explained how we achieved that. If we accidentally add it again, as 269#note_614026 explained, it will blow up.

The style-lint issues aren't introduced here, so merged and releasing 🚀

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

This shouldn't happen in Dashboard but Drupal CMS.

Thanks!

I'm assigning this to me, but collaboration is more than welcome.

Paired with @wimleers on this, and started a new MR:

• We confirmed the problem is when serializing components.
• We found out that this happens when caching the form.
• We found out that actually we are setting the component in the form, but it's never used. So just removed the component for the form.
• TODO: We should be able to serialize the component anyway, because it might be actually needed (follow-up material?) For that we experimented with setting a flag in VersionedConfigEntityBase::__sleep and based on that do an early return in VersionedConfigEntityBase::set.
• TODO: Drafted an initial test for serializing the form, but didn't get that to fail with HEAD as it should (WIP).

We used https://github.com/EdeMeijer/serialize-debugger to actually find what triggered the serialization 😻

The problem is actually happening when trying to set the settings on \Drupal\Core\Config\Entity\ConfigEntityBase::__sleep when serializing.

Thanks!

@mglaman is the only maintainer listed in CODEOWNERS for the Page component, so assigning to him.

Suggested merge message:

            
              
              
              🐛
              Media library broken on 11.2 with Gin 5
                Active
              
             fix(Page): Can't select media from library in Canvas when Gin is installed

By: mglaman
By: wim leers
By: larowlan
By: mandclu
By: gábor hojtsy
By: penyaskito
By: lauriii

Yes, the twig visitor that we have for handling props/slots in the preview doesn't handle includes.
e.g. The twig template is compiled to something like

        if ((($context["variant"] ?? null) == "compact")) {
            // line 2
            yield "    ";
            yield from $this->load((CoreExtension::getAttribute($this->env, $this->source, ($context["componentMetadata"] ?? null), "path", [], "any", false, false, true, 2) . "/test-card-compact.twig"), 2)->unwrap()->yield(CoreExtension::toArray(["attributes" =>             // line 3
($context["attributes"] ?? null), "header" =>             // line 4
($context["header"] ?? null)]));
        } else {
            // line 7
            yield "  <div ";
            yield $this->extensions['Drupal\Core\Template\TwigExtension']->escapeFilter($this->env, ($context["attributes"] ?? null), "html", null, true);
            yield ">
    <h2 class=\"component--my-card__header\">";
            // line 8
            if ((isset($context["canvas_is_preview"]) && $context["canvas_is_preview"]) && array_key_exists("canvas_uuid", $context)) {
                if (array_key_exists("canvas_slot_ids", $context) && in_array("header", $context["canvas_slot_ids"], TRUE)) {
                    yield \sprintf('<!-- canvas-slot-%s-%s/%s -->', "start", $context["canvas_uuid"], "header");
                } else {
                    yield \sprintf('<!-- canvas-prop-%s-%s/%s -->', "start", $context["canvas_uuid"], "header");
                }
            }

So not only slots "marks" won't work, but I assume any props used inside might not work either (or at least cannot be updated without a postback for refreshing the preview).

I don't think we can add a test in an easy way here. Not worth it, maybe we can do something when we have ✨ Drupal CMS variant Active .

Updated steps to reproduce. I also missed the steps even it was implicit on the IS, but not _that_ explicit.

For some reason I don't know yet, line lengths weren't checked as expected.
Do we want to force that in the Canvas codebase?

penyaskito → created an issue.

Created ✨ Compatibility with Gin 6.x Active for 6.x branch + updated MR!171 with the proper issue references.

penyaskito → created an issue.

Discussed with @lauriii:

• As in the Gin related issue apparently there are two different related bugs and that's causing confusion, created 🐛 Gin WSODs when is not active anymore after a custom theme negotiator swaps the theme Needs review in Gin issue queue with the MR.
• Let's be pragmatic and merge !171 in Canvas. We should add a @todo referencing that new issue (this should be fixed upstream), and another one referencing 📌 Allow XB to be used on all node types Active , as if this stays we need to fix this for any other entity types/bundles that are editable in Canvas.

Needs work for the second point.
Also tagged as Needs followup, because testing this we found a different error that prevents Canvas to work at all with the Gin 6.x branch, which is actually Canvas fault. We should fix this once Gin 6.0.0 is released or if Gin goes into Core (see 🌱 [META] Gin 6.x Active )

Moved MR to this new issue for avoiding confusion.

penyaskito → changed the visibility of the branch 3539391-gin-active-not-active to hidden.

Created 🐛 Gin WSODs when is not active anymore after a custom theme negotiator swaps the theme Needs review for disambiguation of both issues.

penyaskito → created an issue.

https://git.drupalcode.org/project/canvas/-/jobs/7009122#L442

Updated IS.

CR changes: Fixed SDC acronym definition, changed target audience to include Themers, included reference page builders and a note that it's each page builder choice to decide if they respect this metadata.

I'm still not 100% happy with the CR, but RTBC to put this in front of nod_ and pdureau eyes.

Added a test. I can't merge this as I'm not code-owner for PropSource, so assigning to Wim

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

penyaskito → changed the visibility of the branch 1.x to hidden.

Thanks!

#4 suggested reusing CanvasResourceLinkCollection, but

1. would change the data contract expected by the client;
2. would require some string mangling for entity types with bundles on the client;
3. the create url wouldn't actually be used anyway;

so I think this is good enough. Assigning to Wim for confirmation in case I'm missing something here.

penyaskito → created an issue. See original summary → .

This looks great overall, but we lack tests for the update path and some minor issues.

Sadly we don't have any php tests for the component library, so should be fine (but sad) not adding that coverage here.

Thanks!