🇺🇸United States @dsnopek

Alright, allowing enabling a pager where none was configured is a bad idea, because there's no way to choose the configuration for the pager.

So, instead, I've reworked this to require the View have a configured pager, but allow setting it to be disabled by default.

Here's a patch to implement

dsnopek → created an issue.

Thanks! Merged :-)

benjifisher → credited dsnopek → .

Thanks for testing! Merged :-)

Here's a patch that fixes it in my testing

dsnopek → created an issue.

mcdruid → credited dsnopek → .

Thanks! Merged :-)

Here's a patch that puts the initial focus on the dialog close button, and outlines the link for the currently previewed block.

dsnopek → created an issue.

Reported to work in testing! Committing :-)

Here's a patch - however, I have no way to test it personally

dsnopek → created an issue.

There is some extra stuff in it though, there are some panopoly_media files included in the patch.

Eep! Sorry about that. It looks like some changes related to #3467225: Switch back to checkboxes on media library → snuck in there.

Here's a clean patch.

Here's a patch that I think should fix the PHP warnings.

However, related to this bit:

When following the steps in #6 while using the previous patch, the initial preview in the choose block dialog and in the configure dialog doesn't show any items. When you make any change to the config that causes preview to fire again, the items display as expected.

Unfortunately, I wasn't able to reproduce this: I'm seeing the preview with items in the choose block dialog, and in the configure dialog before making any changes.

dsnopek → created an issue.

Merged!

Thanks! Here's a patch that fixes the issue described in #6 in my testing.

However, it's a rather large change, so this really needs to be tested quite a bit!

Here's a new patch that fixes the issues described in #3 in my testing. Please let me know if it works for you!

Here's a patch that fixes this in my limited testing - please let me know if it works for you!

And here's a test build which will hopefully show no regressions: https://gitlab.com/dsnopek/panopoly/-/jobs/7408713134

Here is the patch!

dsnopek → created an issue.

dsnopek → created an issue.

dsnopek → created an issue.

Ack, I didn't see your patch, and just fixed this one independently.

Thanks! It works great in my testing :-)

Here's a new version of this patch for Drupal 10.3.

Here's a version of the latest patch for Drupal 10.3 without the tests, for folks who want to include this in their composer.json.

I've spent a little time thinking about this, and I don't think we should try to make the MagicBlock try to work as if the View was using the underlying entity type, given that Search API indexes can hold multiple entities types. I think that MagicBlock should only be able to work on Views of a single particular entity type.

So, I think we should focus on fixing the fatal error:

The error is happening because Search API views don't have a base entity type, so they return FALSE for $this->view->getBaseEntityType().

If that function returns FALSE, then MagicBlock shouldn't allow changing the display type.

Ideally, it'd be great if we could remove the "Display Type" option when configuring a view that isn't on an entity type, so that site builders can't even enable this option for a View that it wouldn't work on.

This is a core issue - the latest patch on 🐛 Problem with action path for embedded forms Needs work fixes it for me

This problem still exists for Drupal 7, and the patch in #9 still fixes it for me.

The patch on #45 looks good to me, and has been working fine on some production sites for a while.

Merged!

Here's the patch

dsnopek → created an issue.

Here's a re-rolled patch

Merged!

Thanks! Merged

Patch is attached

dsnopek → created an issue.

@cboyden confirmed this was working outside of the issue.

Merged!

Here's a patch that fixes it in my testing.

Please let me know if it works for you!

In Panopoly 1.x there was the idea of an admin title, which was separate from the block/widget title and was displayed in the Add Content dialog. Maybe this is just missing?

There are definitely a bunch of bugs here! But it's not quite so bad as not having a field for admin title:

The bugs I'm seeing in my testing are:

1. The "Admin title" field is not required. So, it allows you to save the block without even filling it out, giving the block an empty admin title. I assume this is what's happening in your case - perhaps you just didn't scroll down far enough to see the "Admin title" field? Anyway, this field should either be required, or have some reasonable default.
2. When using a block that's already reusable, it's not showing the "Admin title" field anymore, and saving will clear it. We should continue to show the "Admin title" field (although, we can no longer toggle reusable off once it's been turned on). I assume the reason that saving the block is clearing the title is because the field is missing.

Committed. Thanks!

Merged!

dsnopek → created an issue.

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

Merged. Thanks!

First iteration merged!

dsnopek → created an issue.

@Mysdiir's MR has been merged into a new 3.0.x branch. So, the plan is that 3.0.x is the version that supports CKEditor 5 :-)

@drupatz:

Hi, using the element <drupalbreaks> causes the cke adding   and <p> tags, resulting in <p><drupalbreaks>&nbsp;</drupalbreaks></p> -- in my point of view this isn't a sober solution.

As @Mysdiir mentions, if you enable the Drupalbreaks filter on your text format, it will convert the <drupalbreaks>&nbsp;</drupalbreaks></p> to <!--break-->.

But I agree with you that the additional <p> is unnessesary.

I've just pushed a commit that will get rid of the extra <p> too.

I'm thinking of merging this into a new branch (perhaps 2.1.x or 3.0.x -- I'm not sure yet) so that it's easier to pull into a site and test. We're not fully committed to this approach until we make a proper release. :-)

Thanks!

I've updated the code on the GitLab issue fork such that it's now working in my testing :-)

@Mysdiir Thanks!

So that it'd be easier to put review comments on the diff I made an MR and added quite a few notes:

https://git.drupalcode.org/project/ckeditor_drupalbreaks/-/merge_requests/3

Please let me know if you're interested in making those changes. Otherwise, I'd be happy to take this over and finish it up :-)

This makes sense to me! While this may lead to slight change in behavior with weird inputs, it seems to me that the outputs it would generate are a little bit more sensible than previously, so I think this is OK.

It looks like you're trying to push to the main project repository, rather than the issue fork. Try changing your remote to git@git.drupal.org:issue/ckeditor_drupalbreaks-3395704.git - the git commands would be something like:

git remote set-url origin git@git.drupal.org:issue/ckeditor_drupalbreaks-3395704.git

But if all else fails, you can always attach a patch the old-fashioned way :-)

Here's an initial patch

dsnopek → created an issue.

I've merged the automated patch, plus some minor changes: switching the \Drupal::service('extension.list.module')->getPath(...) to $this->getModulePath(), and changing the fakeobjects version constraint to ^1.2 per #9.

There's some good stuff in the MR from @Mysdiir which updates the plugin to work with CKEditor 5. However, (1) we should probably do the CKEditor 5 update in a different branch (2.0.x can stay compatible with Drupal 8, 9 and 10, with CKEditor 4 -- for which we need just the simple changes merged here), and (2) there's a bunch of miscellaneous other changes in there (.idea/, node_modules/, etc) and so it can't be merged as is.

@Mysdiir: I'd recommend opening a new issue and reducing your MR to just the changes necessary to update to CKEditor 5

Thanks!

I tested the Drupal 9.5 patch from #22, and it worked for me locally! I'm not sure why it didn't work for @irene_dobbs?

However, it looks like this also needs to update web/core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php because there's a test failure in CI.

dsnopek → created an issue.

Hm. This doesn't appear to be the case anymore!

This was fixed some time ago

Merged. Thanks!

Merged. Thanks!

Merged!

Committed

Here's the patch!

dsnopek → created an issue.

This issue appears to be fixed in 8.x-1.3 by #3159450: Module does not work in Drupal 9.0 →

Here's the patch!

dsnopek → created an issue.

Here's a patch that fixes it in my testing!

dsnopek → created an issue.

Here's a re-roll of patch #83, but where the cache entries expire after 1 hour, in order to work around the cache ballooning issue.

Attached is a patch to fix!

dsnopek → created an issue.

It looks like an instance of $config['expanded'] got converted to isset($config['expanded']) when it shouldn't have been (since this issue is just about $config['parent_mlid']

Here's a patch that pulls in the patch from 🐛 Make modal iframe tab accessible Needs review and additionally:

1. Removes hidden TAB'able items from the modal
2. Shows a highlight when focused on the image thumbnails
3. Allowing selecting and de-selecting images from the keyboard

dsnopek → created an issue.

Alright, this is hopefully my last patch before getting some review :-)

In this version, it starts keyboard focus on the modal's close button when it opens, and allows focus to freely flow through the elements on the iframe and back up to the close button (both TAB'ing forward and SHIFT TAB'ing backwards).

I think this is everything necessary to say the modal iframe is tab accessible :-)

New patch fixes another related issue: currently, if you TAB to the "Browse" button (the one that opens the modal) and press ENTER, the focus will momentarily move to the modal, and then be sucked back to the "Browse" button which is under the model.

With the patch, the focus will move into the modal (although, not in the most ideal place - it focuses the iframe itself, when it'd be better on it's first child or the modal's close button).