Good idea! Put in a suggestion.
I think we should look at RevertOverridesForm too, that might be even more confusing than this one
Changes look great. I'm happy with #49, it feels like the lowest impact solution for a situation that should be rare outside of tests.
I've popped one thought in about a hook_requirements implementation, so leaving in needs review for people to have a think about it. I don't feel strongly about this, if its decided we don't want it then this is ready to be RTBC
It would be good to get the 11.x MR up to date with these changes too, fixes should go there first :). Thanks!
I don't feel strongly about this one. I'm a soft +1 for ComponentType. In a ComponentSource (using it's current name), nothing is actually being sourced from anywhere. At this point it's handling the render and data transformation layers for use inside XB. This makes it feel more like a type to me, and I think @effulgentsia points at the bottom of the IS outline this in a really clean way that makes sense to me.
At the moment we decocate cores plugin providers (BlockManager & ComponentPluginManager) to create/update XB component entities from existing plugins. We don't currently use any particular term or interface for these, but to me this is what should be considered a Component Source. Components are being sourced from another system like block or SDC currently, and LB and paragraphs in the future.
Naming is hard!
This should only apply to SectionStorage plugins that DO NOT extend SectionStorageBase and instead implement the SectionStorageInterface.
I searched through Drupal contrib projects and can't find any examples. I agree that we should add CacheableDependencyInterface to SimpleConfigSectionStorage and remove the if. I'll start on that and see how tests look
Thanks for raising an issue!
I've tried to replicate this with 11.x and I'm unable to see this issue. Can you please try this with just the latest version of Drupal and update the steps to replicate if you're able to get this to happen again?
I've tested this on the latest version of 11.x and I'm unable to replicate this. Tested on many different screen sizes. Can you please test if this is still an issue and update the steps the replicate?
The original reporter mentioned in #9 that this can be closed and this currently still sits as a support request, so I'm going to close this issue.
If you have any thoughts about improving this, feel free to open a feature request and discussions can continue there.
Thanks!
It has been close to a year with no further updates on this one, and most of the recent comments talk about incompatibility with Gin. I'm marking this one as closed.
Please feel free to reopen or open a new issue if you can replicate this with just core.
Thanks!
Test coverage added and tests are green. I'd be interested to get a second set up eyes on this one. Mainly around this comment.
I'm a little iffy of how many places we are calling ::hasLayout. I wonder if we can put that in a central place instead.
No problem! Just thought I'd flag it so it didn't get blocked later when a committed reviewed this :)
Changes look good! And agree that since the work is already done, tests are passing and it's a test module that it's ok to do it all in one issue.
It's best not to RTBC issues that you've worked on and instead have another person review and mark it as RTBC.
In this case I've reviewed and I'm happy for this to be RTBC.
The only thoughts I had around this one is that it could be worth updating the title and/or description to make to clear that the existing hooks have also been split out into dedicated files. When I first reviewed the MR I was confused at the changes, since I only expected to see the functions in the .module file moved into hooks
It's best not to RTBC issues that you've worked on and instead have another person review and mark it as RTBC.
In this case I've reviewed and I'm happy for this to be RTBC.
I don't believe this is a Layout Builder issue, it's just that Layout builder is exposing this.
If I understand it correctly, the issue thats raised here is that too much sample text is being generated by textarea fields when placed in Layout Builders default layout. We can trace this down to either StringLongItem::generateSampleValue or TextItemBase::generateSampleValue (based on field type) that both call Random::paragraphs() without a parameter, which means it will generate 12 paragraphs of sample text.
As quietone mentioned, there isn't a lot of activity here to explore making this change at the moment, but just leaving this here as a reference point in case people are interested in exploring this change.
The patch changes from #64 need to be incorperated in the MR(s) listed as part of this issue.
I've added test coverage, and also fixed issues where this wasn't working on the default layout or on nodes that did not have overridden layouts (Yay for tests)
Lets see how the pipeline goes here. I'm a little iffy of how many places we are calling ->hasLayout. I wonder if we can put that in a central place instead.
This absolutely needs test coverage.
I haven't thoroughly looked into this issue yet, would it would to have a test module that provides a layout, add that layout to a page, then uninstall that module and verify everything still works as expected? If I get a chance I'll look into this.
This has gone through multiple rounds of review, all threads have been resolved. I think it's ready to be RTBC.
MR is looking good. I've done a couple of code reviews and left a few minor comments. re: #37 I'm ok with the API change as outlined in the MR, but I'll leave the tag for now just in the case the other LB maintainers want to weigh in.
griffynh → credited danielveza → .
Made some progress on this today, assigning to me to wrap up
Just to keep this one moving along, I've started to update some of the tests, had a lot of failures before, lets see how this run goes
danielveza → made their first commit to this issue’s fork.
Addressed feedback, tests are green
Im pretty interested in following the ContentTemplate work, so I figured I'd make a start on this one based on the IS and the discussion from #2. I've pushed up a POC of the existing work I've done so far, which now adds config dependencies for fields to the content template entities. This has test coverage, but still additional work to be done around code cleanup and removing the legacy code from `ComponentTreeItemTest`
danielveza → made their first commit to this issue’s fork.
This has been partially covered above and may be part of the video from #19, but I just wanted to have my thoughts in written format from pain points experienced with Layout Builder and overrides. As flagged by this being a stable blocker so I'm happy this is on the radar, I think its something that XB needs to get right (to the best of our ability) from the start. I'm going to use Layout Builder terms in this comment but the same logic should apply to their XB equivalent.
Note that this is going to be a braindump and going to be a bit rambly.
Layout Builder is a fantastic tool, and the contrib community around has built tools to further support how it works. Where Layout Builder falls over however, is getting updates from the default template to nodes that have been overridden.
Lets say we have a content type with 5 sections in it's default template, and 10,000 nodes of that content type where most if not all have been overridden to add additional content on a per node basis. Now we need to add a new block to section 3 for legal or compliance reasons.
This new block is not going to appear on any of the overridden nodes, so we need to write a complex update hook to add it everywhere. However there are still problems here. We can't guarantee that section has not been deleted or moved on any of the overridden nodes, so we need to take that into account. Layout Builder sections don't have UUIDs ( ✨ Add UUID to sections Needs work , so we can't even look up the section during the update process with confidence. This is very flaky so we tend not to do it.
Because of this on some new projects I've started to create blocks that renders a collection of fields/blocks, so the Layout builder content now looks like this.
-- Top content (Collection of blocks in a single plugin)
-- Layout Builder content
-- Bottom content (Collection of blocks in a single plugin)
This means the editors lose some control over what parts of the page they can edit (Only the middle section), but allows us to add new content to the top or bottom blocks that will automatically apply to all nodes, regardless on if they're overriden or not.
This leads us to the second problem, how do we add new Sections to the default template and have them flow through to the overridden nodes in the correct place. Again this is a problem in Layout Builder and something that should be considered for XB.
TLDR; Updating overriden content in Layout Builder from the default template is painful. We should take the lessons we've learnt from Layout Builder and apply them to XB from the start.
danielveza → made their first commit to this issue’s fork.
Ah actually. While I was prepping the MR for this it wasn't as easy as I was initially thinking when I opened this because we can't override the click element, and other approaches I was thinking about wasn't as clean as I was hoping for.
Postponing this one so I can think about it a bit more.
danielveza → created an issue.
Feedback addressed and tests are green again
Changes look good, the only thing I noticed missing from the original patch were the changes to BasicAuthResourceTestTrait::getAuthenticationRequestOptions. Should they be included in this?
Other that that it looks good and is ready to be RTBC, just checking in on that one first.
Ran a test of this from the latest 11.x branch. I implemented validateConfigurationForm on the TwoColumnLayout class that set a error, and its displaying correctly.
This appears to not be an issue and it has been many years since the last update, so I'm closing this.
Thanks for raising an issue!
Layout builder in core is used specifically for managing the display of a page, there isn't support for the edit form. So I'm closing this as a won't fix.
Regarding the Drupal updates, there won't be the full module rewrites that was required like the Drupal 7 -> 8 updates. There tends to just be minor tweaks needed between major Drupal versions now to keep the modules up to date and compatible with the new versions of Drupal.
Drupal has a slack community → where questions like this can be asked if more information/help is needed :)
CR created: https://www.drupal.org/node/3513877 →
MR is up for this and green. Moving to review.
I imagine this will need a CR?
How would we want to handle the removal of this code in D12? Do we wait for this to be committed and open a follow up?
danielveza → created an issue.
Thanks for raising an issue!
I'm having a hard time understanding what would be required here. Could you please update the issue with specific steps to replicate & information about what you'd like to see and what the desired outcome would be?
If this is required on a per site basis it can be done with a form alter (See LB Claro for an example)
Since this can be done with standard drupal hooks & there hasn't been any updates since 8.6 this doesn't appear to be a highly requested feature. In my opinion this should be closed. Leaving it as postponed for the moment for any others to give thoughts
I agree with what @tim.plunkett has outlined in #18.
Since this is mainly an API module, most of the functionality that would be described in the 'uses' section is already covered in the existing 'about' section.
Other API type modules such as phppass, and the db driver modules also don't have a 'uses' section so there is precedent to excluding this.
Marking as postponed for others to give thoughts before closing.
This has had no updates since 8.6, is this one safe to be closed? Leaving as postponed for the moment for others to give thoughts
After 8 years and no updates since 8.6. I think we can recommend to close this now. Marking as postponed for the moment for others to give thoughts
jct321 → credited danielveza → .
Popped a review with some thoughts.
I think we need to decide the scope of this. Do we want to:
Allow specific attributes to be passed to the pass2 element.
OR
Allow the pass2 element to inherit the properties passed to the password_confirm element.
I'd vote for the 2nd, and maybe open a follow up for the first.
Whats others thoughts?
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 identicalLooks 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.
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.