🇳🇿New Zealand @danielveza

Done a review & there is still a couple of older unresolved threads

Could this please be updated to include the issue summary template, including steps to replicate and what should be seen

Looking again, I think this is a duplicate of 🐛 Formatters for empty fields do not render with layout builder enabled Needs work . I think this one can be closed and effort focused there

Hey @klu, could you please create a merge request from the patch that you've created against the 11.x branch?

There is some documentation here → about how to create merge requests for Drupal issues.

Thanks!

We asked for steps to replicate in #4, and this issue has now been open for 8 months without any further response, so I'm closing this issue.

Thanks for raising an issue, if you find a way to replicate this please feel free to open a new issue.

danielveza → created an issue.

I think personally I'm a -1 on this idea being part of XB core. It sounds like a good contrib module that could be added on top of XB. The IS raises 6 points that we get for free with nodes and others have been raised in comments on this issue. This is extra code & tests to maintain and increases technical debt.

I think XB should be built with node in mind from the start, otherwise we're losing functionality that we currently have with Layout Builder out of the box. Without node support, I think real world adoption will be slower and developers will have less confidence in adding XB to new sites.

For example, we were early adoptors of CKEditor5 in new sites because it was a drop in replacement for CKEditor4 and because of that we were able to catch and fix issues ( https://www.drupal.org/project/drupal/issues/3339400 🐛 Incorrect use of FormattableMarkup in logger messages Fixed , https://www.drupal.org/project/drupal/issues/3280602 🐛 Exceptions for CKEditor 5 plugin definitions containing wildcard tags when PHP is built with libxml 2.9.14 Fixed ) in CKEditor5 before it was stable.

We couldn't be early adoptors of this before it supports node, since we would need to run both Layout Builder and XB side-by-side, which doesn't feel worth it.

I understand that this is being built with the idea of rapid iteration and it's been said that supporting nodes out of the box will be done before the 1.0 release, it just feels like we should be fixing the node related issues as we go rather than kicking the can down the road and saying we'll figure it out later.

I've just hit this on a client site too. Attempting to track down if it's something we should actually fix here or if its a problem with custom code.

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

nuez → credited danielveza → .

it still allows contrib/custom code subclassing rather than copying, hence it still is a de facto API.

I can understand wanting to keep API scope small 👌.

The points you've made make sense to me and it does still achieve the goal of cleaning up the adapter code, so +1 that we merge it

I think this is also works, I just have some thoughts below around the original approach & questions about the new trait :)

Having said that, I prefered the original approach. I think it was a perfectly valid and having the final constructor would help alleviate some of the concerns around making the ImageAdapter non-final.

The new trait is also very specific, it works if the class ONLY needs EntityTypeManager otherwise you need to either mess around or remove the trait to inject other services.

Maybe if we go the trait route we narrow the scope a little and call it something like ImageAdapterTrait?

Left a couple of tiny comments on the MR.

The IS mentions updating CONTRIBUTING.md, I've checked it in the merge branch and I don't think anything needs to be updated :)

Confirming that this cannot be replicated, following the same steps I used when I originally created this issue. I think this can be closed.

I won't RTBC since I did the original work, but confirming that the changes from @omkar-pd look the same as my original changes just with the latest commits from HEAD pulled in

griffynh → credited danielveza → .

danielveza → created an issue. See original summary → .

danielveza → created an issue. See original summary → .

danielveza → created an issue.

My POC didn't take long so it's ok that it didn't make it in.

The reason I thought a feature flag here could be valuable is that we have no guarantee that the core issue is going to get in before XB is considered production ready or early adopters start using it and I'd like to avoid future devs having pain when trying to track down what is most likely a quite annoying performance regression to figure out.

Probably the softer approach for XB rather than the feature flag that removes it is a hook_requirements implementation that runs if JOSN:API is also enabled. I'll open a ticket.

Ah interesting, did a search for the fixed element scoll issues in the Sortable JS queue and came across this issue - https://github.com/SortableJS/Sortable/issues/1582

You are using native browser autoscroll because forceFallback is not set to true in the options. You will have to set forceFallback: true and then set the scrollSensitivity option. But you must use 1.9.0 because this option is broken in the 1.10.0-rc versions.

We are on 1.15 so it's not broken for us, we might just need to experiement with forceFallback? I'm off this for the day but leaving these here so I/someone eles can review and see if thats an option for us

Popped a couple of comments on the MR, I agree we should figure out why dragging is blocked by the nav. I suspect it's something to do with the fixed positioning.

I ran some tests locally and bumping the scrollsensitivity up to 100 or so felt like it made the scrolling experience smoother, since thats accounting for the 60px we are losing from the nav blocking the scroll

Pushed up an alternate approach based on the feature flag idea to 3469516-feature-flag.

It should hopefully prevent us from needing to add this as a runtime dep and stop the JSON:API preformance regressions outlined in #4.

That being said, I also agree with this in #7

>Or better yet: have the JSON:API module add a JSON:API development mode, similar to core's recently added Twig development mode?

We should open a core issue for this, it feels too easy to shoot yourself in the foot without realizing it and degrade your JSON:API performance. (As this issue has proved)

Based on #4, if this does get in we should open a follow up to explore other options that aren't going to have performance regressions for unrelated parts of the site.

My MR is green, the phpcs being out of date will be resolved by 📌 Improve UX of adding new sections Needs review , I don't think that needs to block this from getting in.

The MR does not contain the changes that make the ghost area bigger, as I don't think thats required anymore with the border being applied

Was going to open this issue myself but I see this one is already done. The pipeline is green, so that suggests the change is correct. But as a sanity check I cloned both phpcs files locally and diffed

local-machine:/tmp/test$ diff -s ex-phpcs core-phpcs 
Files ex-phpcs and core-phpcs are identical

Looks good to me! Lets get this in so our pipelines are green again

Nice! I'm glad my suggested approach was correct

kristen pol → credited DanielVeza → .

DanielVeza → created an issue.

I've added a POC of #10 to this MR. Still wrapping my head around how XP is built but it is working well.

I set it up as a seperate MR for the POC but if everyone is happy we could combine them. I've attached a screencast.

DanielVeza → created an issue.

Yes this is still reproducable. Using the same steps to replicate as in the IS on a fresh clone of experience builder. Screeshot below. I've added one hero then dragged it around in the sidebar.

DanielVeza → created an issue.

DanielVeza → created an issue.

I haven't done a comprehensive review of this yet since it's not green, but does the test need to be FunctionalJS? I think we can just do a Functional test here

Hi!

10.2 is now only recieving security coverage, and any further work on this should be done on the main issue for this ( ✨ Allow hiding configured blocks in layout builder Needs work ).

Thanks!

DanielVeza → created an issue.

Could you post the backtrace?

Is there any contrib or custom Layout Builder/view mode modules that might be causing an issue here? Echoing @smustgrave in #8 I think we need to find the cause before we can say if this is the right fix or not. I suspect something else might be at play here.

Rewrote the test to be FunctionalJS that follows the unhappy path

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed.

Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed.

Thanks!

As per #10, it appears that another issue has already provided the required functionality. There has been no further updates to this issue since the last comment over three months ago, so I'm now marking this as closed.

Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

No further updates have been made on this issue since asking for steps in replicate in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

If this one can be replicaated with just core or other people see this, feel free to reopen or open a follow up issue. Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

No further updates have been made on this issue and it was unable to replicated in the last comment over 3 months ago, so I'm now marking this one as closed. Thanks!

This has been open for 5 years with no comments outside of the original post, and no further updates in 3 months since I put thoughts in, so I'm now marking this issue as closed.

Thanks!

No further updates have been provided in over 3 months since this was marked as Postponed (maintainer needs more info). This appears to have been improved since this issue was first opened as per the screenshot in #22, so I'm now marking this as closed.

Thanks!

Solutions for how to achieve this have been posted in this video and it has been over three months since the last update where this was marked as Postponed (maintainer needs more info).

Therefore I'm marking this one as closed. Please feel free to reopen or open a follow up ticket if you think there is still work to be done here.

Thanks!

This issue is now over 4 months old, and no new comments have been added since the contributed module suggestion in #8, so I'm now closing this one.

Thanks!

This issue has been open for over 3 months without a reply so I'm now closing this one.

Thanks!

Ah ok - I tracked this down.

This issue stems from the fact that /taxonomy/term/X is already a view page, that also embeds terms in the view header. The workaround is to disable the taxonomy term view, since it's not needed if you're using LB with a term page.

This feels like a pretty painful spider web to untangle, I wonder if we should add a warning when you enable LB on a vocab display to make sure that either the view is disabled or a filter has been added to the view so exclude this particular vocab

Confirming that I can replicate this on D11, so updating the version.

It's a strange one. 2x pagers render specfically when embedding a views block in LB, but only on taxonomy term pages. An additional mini pager is rendered along with the one that comes from the view.

If you embed a block through the block management on a term without Layout Builder, it works as expected. I changed theme and the issue still happened, so it doesn't appear to be a theme issue.

Spent a bit of time trying to track this down but couldn't manage it. Leaving these notes here to pick up later.

I agree with #25

Thanks for raising an issue! This would need test coverage before it is can be moved to RTBC.

I'm not entirely convinced it is a better UX than the existing setup, but I'd be interested in hearing others opinions on it too

I think asserting the class .block-system-breadcrumb-block is more robus

Don't personally feel strongly about this, can add it in along with the preview test check.

I also personally prefer the approach of adding a few assertions to an existing test

I disagree with this one for a couple of reasons

This issue is in the breadcrumbs block, not Layout Builder, so the test should live in the system module, not Layout Builder
Layout Builder tests are already doing too much in single files and need to be split, so I don't want to add more bloat.

Moving the block is vital to triggering the error, so I think the test needs to move the block.

I'm 50/50 on this one. FunctionalJS tests are slow and I don't think it's needed in this case. I fully understand writing a test that runs the exact steps to trigger the issue, but in this case I think we can be confident that the changes to the build function of the breadcrumbs block also resolve the dragging issue, and they're being tested in this issue.

That being said, I don't feel strongly about this one and I can see the other side of the argument for sure. If someone would want to rewrite it as a FunctionalJS test I wouldn't block it

Tests on MR is green now.

Adding an entirely new BreadcrumbBuilder for this block feels excessive. I propose we simply check if the block is in preview and display the fallback text if it is. This was suggested in #19 and that issue has now landed so this is possible.

Added a MR with this approach with test coverage.

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

Thanks for raising an issue!

Can this be replicated without the contributed module? If not could we please get a backtrace? It may help identify if this is something that should be fixed in core or in lb_plus.

Ran the tests only feature since I couldn't replicate this in #9. Test fail as the steps to replicate have described, so I expect that was an issue in my local testing.

Code changes look good, I was debating if we should add a $this->getRequest()->query->has('destination') check around it, but since no other processing is being done to the destination I think it's safe to just run the remove.

$this->getRequest()->query->remove('destination'); does an unset of the param passed, if it doesn't exist there should be no issue.

This may be resolved by 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed .

Since this needs a contrib module to be able to replicate this, I think it would be good to understand why this is happening with LB plus so we can define if the issue exists in that module or something that should be fixed specifically in core. Just casting things to strings feels fishy to me.

layout_builder.settings was removed in 📌 Replace "Expose all fields as blocks to Layout Builder" configuration with feature flag Active . As far as I understand this is no longer required, so I'm closing this. Please feel free to reopen if I've made a mistake with that.

Thanks!

larowlan → credited DanielVeza → .

DanielVeza → created an issue.

griffynh → credited DanielVeza → .

DanielVeza → created an issue.

Hello! Thank you for raising a very detailed issue.

Could you please confirm that the steps to reproduce in the IS are with a fresh install of Drupal 11? If not can they be updated with the steps to reproduce starting with a fresh install of D11?

I'm asking because I've tried to this and I've been unable to replicate this. The steps I've followed are below:

• Fresh install of a standard D11 site
• Enabled LB
• Added Layout builder to articles
• Added a text field to articles with a default value of "Default text"
• Created a new article and removed the default value from the field
• Went to LB for the article and added the fieldblock for the new field.
• I'm not able to see the label as described in the IS

Thanks!

string|\Stringable makes sense to me and I agree with the comments in the remaining tasks section of the IS.

Tests are still green, no linting issues introduced. Looks good!

griffynh → credited DanielVeza → .

DanielVeza → created an issue. See original summary → .

smustgrave → credited DanielVeza → .

DanielVeza → created an issue.

We are reviewing this again as part of the Bug Smash Initiative. This has had no updates in over a year and earlier reviews suggest that this has been fixed by other issues, so I'm now marking this issue as closed.

Unless I'm looking at the wrong place, the patches in #5 & #10 both take different approaches. I think we need some clarity here about which approach is the correct one and sync them up.

I totally understand the thoughts behind this, personally I'm not 100% sure about this idea. This seems like it would be highly disruptive to both contrib and to anyone that has applied custom styling to LB.

I think this could live in contrib so it can be an opt in for people who want it.

Thank you all! I'm happy to join the team :)

This has been inactive for 3 months since the last comment that asked for more replication steps. I'm marking this as closed. Thanks!

griffynh → credited DanielVeza → .

I was concerned the new approach could stop the message from showing when we do want it to display, but I've checked all child classes of EntityDisplayFormBase and the only one that sets redirects in the form state is LayoutBuilderEntityViewDisplayForm.

I prefer the approach taken here to my original MR & it's flexible enough to be used in the future if something like this pops up again. So +1 from me

Confirming that I'm still interested in being a maintainer and that I've read the required documentation.

Thank you for the kind words and support!

tim.plunkett → credited DanielVeza → .

No further updates in 3 months, examples have been provided on how to do this. I'm now marking this issue as closed.

Thanks!

Correct screenshot 🤦

I've tested this on a fresh install of Drupal 11 and this issue doesn't not exist.

Since this issue is over 3 years old and the issue appears to be resolved, I'm marking this as closed.

The steps to replicate this in the IS & in #3 both involve the use of Layout Builder Restrictions.

Can we please get some steps to replicate this with a fresh install of D11 and no contributed modules?

Thanks for raising an issue! In my opinion this is a duplicate issue of 🐛 System Menu blocks do not have attributes Needs work . That issue has an open MR and some further progress, so I'm closing this one as a duplicate.

Thanks!