Account created on 19 July 2011, over 14 years ago
#

Merge Requests

More

Recent comments

🇦🇺Australia pameeela

Merged to 1.x.

🇦🇺Australia pameeela

Tested and works as designed, added to the merge train.

🇦🇺Australia pameeela

Merged the in-progress work just to avoid ongoing conflicts but will keep going here.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Works as designed, added to the merge train!

🇦🇺Australia pameeela

This was covered in 📌 Add useful default content to starter Active .

🇦🇺Australia pameeela

Added change record.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela made their first commit to this issue’s fork.

🇦🇺Australia pameeela

Haven't done this in a while so I'm sure I did something wrong, but I tested and it does install as expected.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Removed the component and also removed the component READMEs which are just dummy text. We might create READMEs for the components, but they should contain relevant content and not dummy text. Out of scope but also harmless.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

To the surprise of no one, the tests are failing on this.

The manual install works, so I'm not sure what to do here.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

@charles belov the MR is for 11.x, considering how big the changes are it’s not surprising if it doesn’t apply to 11.3.x (or possibly it’s too large so it’s failing, I’m not sure). Since it’s RTBC already, additional testing is a bonus but not critical so it will probably be best to wait until it’s merged.

🇦🇺Australia pameeela

This should be postponed until the other issue is merged, my bad. There is nothing to review just yet as we can’t work on this until it’s committed.

🇦🇺Australia pameeela

Created 🐛 Fix styling of link autocomplete with long titles Active for the CSS issue and removed it from the IS.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Duplicate of 📌 Mark Navigation as a stable module Active

🇦🇺Australia pameeela

Using trim on url also works. But I still don't know how to fix the empty check.

🇦🇺Australia pameeela

I pushed a fix, but I also noticed that the if not empty check doesn't actually work. I guess that's a follow up.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

It's quite unfortunate that this recipe is called drupal_cms_remote_video..... but not worth holding things up. Tested manually and it looks good.

🇦🇺Australia pameeela

Thanks @ckrina for weighing in, I think we can close this.

🇦🇺Australia pameeela

@phenaproxima asked me to weigh in on this, but I have no opinion whatsoever. I will leave it to those who know!

🇦🇺Australia pameeela

This didn't seem to make any difference in the rendering, but spoke to @lauriii and added the height and width to each and it seems to be working as expected -- but @balintbrews can you do a quick review?

🇦🇺Australia pameeela

Committed to 1.x.

🇦🇺Australia pameeela

Looks good to me.

🇦🇺Australia pameeela

Merged, unfortunately this changes some component versions, I will try to update those today.

🇦🇺Australia pameeela

Looks good, I only made small changes to remove the example value for bg color so it defaults to none, because that is the most common scenario.

🇦🇺Australia pameeela

Ooof sorry to hold this up so long! Looking now.

🇦🇺Australia pameeela

That's very weird, I tested in that order and it still didn't work but I'll create that follow up so we can get to the bottom of it.

🇦🇺Australia pameeela

When the text format includes the Drupal Media Library toolbar item and the "Embed Media" text format filter is active, a media entity can be inserted and, using its CKEditor5 balloon interface, can be hyperlinked using the same autocomplete interface.

I tested this and it doesn't work at all. Not sure about the config issue you mentioned @mark_fullmer as I was testing with Umami so it was already configured.

The media markup got mangled by the link, the image still rendered fine but it is not hyperlinked. This still feels like something to fix in a follow up though? The functionality is still quite limited and not on par with Linkit, so I think it's ok to have a few known issues. It will be so much easier to resolve them if we can do it in separate issues as the thread here is already so hard to follow!

🇦🇺Australia pameeela

I think all of those items could be handled as follow ups? None seem super critical and I think this is worth landing with a few known edge-case issues.

But I don't think I understand this one:

If the referenced node has a URL alias, that alias is rendered.

Is this not what happens in the screencast I added? Or if not what am I missing?

🇦🇺Australia pameeela

I think hiding it altogether when there is only one option would be a major UX improvement. It simplifies the form by a lot and for simple workflows it's overwhelmingly likely that it matches the publish/unpublish in the datetime labels. So I think showing it even as plain text is unnecessary.

I just tested it out to see, and I think it still ends up with the same confusion mentioned in #8.

Compare the three possibiliies.

🇦🇺Australia pameeela

Why not just hide the publish or unpublish state fields entirely if there's only one choice?

+100 to this! I came to the same conclusion after reading through the comments.

🇦🇺Australia pameeela

In our case it was because the module was being enabled in a recipe that also contained the node edit form, and the fields were missing from that -- so definitely an issue in our config.

I do think it's strange to expose these values on every node, when most of the time it would be published/unpublished -- but that's a separate conversation!

🇦🇺Australia pameeela

pameeela made their first commit to this issue’s fork.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Postponed until 🐛 Do not use Tailwind CSS class names in prop enum values Active is merged though.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Looked back at the original media stuff and looks like the plan was to add optional support for this eventually. I'm not sure that it needs to be optional anymore, I don't see any major issues with having it by default.

I assume we can just add a dependency on the core recipe....

🇦🇺Australia pameeela

Taking a look now...

🇦🇺Australia pameeela

@quietone indeed the size of this MR makes it quite difficult to work with. I would say that updating references to Gin in comments can be done in a follow up mostly for that reason!

🇦🇺Australia pameeela

Along with general testing, I tested the feature flags and all worked as expected:

  1. Login form defaults to use admin theme
  2. Setting this to false uses Olivero
  3. No sticky actions by default
  4. Setting this to content_only enables them on the node forms
  5. Setting this to always enables this on all forms

Overall I am in awe of the work that has been done on this so quickly @jurgenhaas and @saschaeggi and whoever else was involved in this superhuman effort :)

🇦🇺Australia pameeela

I got the same as #9 but resolved it by uninstalling the Toolbar module. Then you also will want to enable Navigation.

Is it enough just to document that this is needed if you are looking to switch your site over?

🇦🇺Australia pameeela

There was no mention of updating user.module in the original issue, so maybe I'm missing something obvious but thought it was worth asking.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

@balintbrews I see, thank you for sorting that! So the grouping is still something we do ourselves, but if done this way, prettier will not undo it.

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.

🇦🇺Australia pameeela

Worth noting the user testing was done using the screenshot at https://www.drupal.org/files/issues/2025-11-10/drupal-content-options-sc... -- it was not testing this menu specifically.

Re-posting from Slack, I don't have strong thoughts on whether to rename it to CMS or not but I do feel that the link to Pages is pretty critical as currently it is extremely difficult to figure out how to find Canvas pages. I think this issue is at least major for usability.

🇦🇺Australia pameeela

I've updated it to remove the year, I don't think it's adding much value and will be outdated already soon!

🇦🇺Australia pameeela

Just capturing my comments from Slack, this seems to undo the grouping that we introduced in 📌 Group Tailwind classes in Twig for readability Needs review so just want to make sure we are all on the same page. I've asked @balintbrews to take a look and let us know what he recommends.

🇦🇺Australia pameeela

Gave this a manual test with the latest changes, looking good for the basics but I am not sure what edge cases also might benefit from testing.

🇦🇺Australia pameeela

I think for the reasons that @phenaproxima outlined, recommending this:

composer create-project drupal/cms
cd web
php -S localhost:8888 .ht.router.php

Is too fragile. In testing this locally, I get a warning about OPcode caching not being enabled. Since that is the default, I think we can assume most users will have the same. You can proceed anyway if you scroll all the way down and click on that option, but I don't think it's a great intro to Drupal CMS! And this is effectively best case scenario since I happen to have everything else.

But because of this I agree that the recommended command is not really correct for now, will raise it with the DA.

🇦🇺Australia pameeela

Updated the message as well as all of the links, they were definitely not helpful! I've created a shortlink directly to the install pathways page and linked that as the first thing. That gives users an overview of the recommended options so they can choose one.

I still do think there's something awkward about promoting the vanilla composer-create command as the primary way to download Drupal CMS but I also think that's a separate conversation with the DA about how we message it.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Spoke to @lauriii about this and he agreed that we should avoid quick-start here. For similar reasons that I listed but he also pointed out that Canvas is unlikely to work at all, and indeed it crashes out almost immediately! (It works better with .ht.router.php but this is overall a very poor experience.)

So I think we are limited to updating the existing message possibly to make it more clear what the pathways are? I do also think that having this command be the recommended way to get Drupal CMS makes it slightly awkward, but the user guide does clearly outline the options from here.

🇦🇺Australia pameeela

MR with the config actions, but I am guessing @phenaproxima will want to add a test :)

🇦🇺Australia pameeela

We should just fix this for now anyway.

🇦🇺Australia pameeela

Yeah I guess this is glossing over the problem anyway which is that you can set it up in a broken way.

There are two things that could be addressed:

  1. It's possible to have a broken form and not know it, if the fields are hidden
  2. It should be possible to set a default per entity type so that you can hide the fields
🇦🇺Australia pameeela

If the patch in #3507932: Publish_state and unpublish_state value is coming empty is accepted, we don't need to make this change.

🇦🇺Australia pameeela

We just figured out scheduler was broken in Drupal CMS because the form fields were hidden. I think this patch makes a lot of sense to avoid having to expose two new fields on every node, if you are happy with the default values. Tested that it worked so marking RTBC but I don't have a deep understanding of this module so I understand if this fallback option is not desired.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

pameeela created an issue.

🇦🇺Australia pameeela

Another minor thing, the messaging "Congratulations, you’ve installed Drupal CMS!" probably needs to be changed regardless. You haven't actually installed Drupal CMS in the way that we generally mean it, this refers only to the composer install step -- which is somewhat trivial (and probably not that worthy of congratulations!).

The core message is a bit more clear:
Congratulations, you’ve installed the Drupal codebase, from the drupal/recommended-project template!

So we can at least update it to make it clear that all you've done is install the codebase?

🇦🇺Australia pameeela

I think we are good here, since 🐛 PHP 8.4 implicit type deprecations Active is merged and @webchick confirmed at #3539787-8: PHP 8.4 implicit type deprecations that this resolves the issue.

🇦🇺Australia pameeela

Oh, hey @webchick! Great to see you here, I thought I was hallucinating when I saw your name in my inbox :)

Agree this could be improved, but I am not sure about using quick-start. I use it a lot for testing, and in my experience, the "frozen page" issue documented here happens every time and sometimes very quickly, to the point that I don't even bother with it and go straight to the php -S localhost:8888 .ht.router.php workaround post-install. I wonder if this is unusual or others have had the same experience with it?

The other concerns are minor compared to that one, and it might not matter that much anyway since I don't think this is a commonly used pathway.

If we are going to recommend it though, we should definitely recommend installing Byte rather than Starter (which you get now) because Starter is not a good look for evaluators. That's easy enough though.

The last thing is just adding a disclaimer about it being good for demo/trial purposes? The messaging now reads as though you can either use quick-start or ddev, but they are not the same. The fact that it's sqlite means it's pretty limited in utility, until we solve that problem for the launcher at least!

🇦🇺Australia pameeela

@catch sounds like a worthy effort but I am not sure whether there is an actionable change?

I wonder if suing the variable version of the font might be neutral or possibly lighter.

I am afraid I don't know what this means!

🇦🇺Australia pameeela

It looks good to me as far as the readability but I can't really speak to accuracy without doing a deep dive!

🇦🇺Australia pameeela

On the merge train for 1.x.

Production build 0.71.5 2024