This pull request has conflicts, please resolve those before we can evaluate the pull request.

@TLWebdesign any chance of fixing the conflicts and moving out of draft status so it can be tested in time for the 6.2 release please

@brianteeman i think i fixed it now.

I have tested this item ✅ successfully on 008aaaf

I have successfully tested this PR. Seems to work great now! (hug)

Thanks @TLWebdesign!

I have tested this item ✅ successfully on 008aaaf

I have successfully tested this PR. Seems to work great now! (hug)

Thanks @TLWebdesign!

Nice work @TLWebdesign ! Found an inconsistent behaviour when "inherit" or "inherit all" is set on a parent, the subtree items of type url/heading/alias/separator were skipped during force-checking, leaving them unchecked while their siblings
appeared checked. But after save and reload all are checked.

Additionally, switching back to "no inheritance" left these items stuck in their previous checked state with no way
to manually correct it since they are always disabled. Everything is displayed consistently again only after reloading the page.

This is happening on chrome and firefox - I made you a screen-record.

The changes in my review fixes this inconsistency.

Even though it makes perfect sense to me to mark these types (like alias, url etc.) as well in conjunction with the new "Inherit" option. I still find it confusing that they are marked in the exact same way, given that it is quite obviously impossible to load a module onto them (since there is, after all, no actual page to load a module for these types).

Instead of this...

I personally would prefer something like this

I opened a PR on your Repo for this additional style changes,

TLWebdesign#1

Now you can decide, if you only want the js fix from the review or both :)

Nice work @TLWebdesign ! Found an inconsistent behaviour when "inherit" or "inherit all" is set on a parent, the subtree items of type url/heading/alias/separator were skipped during force-checking, leaving them unchecked while their siblings appeared checked. But after save and reload all are checked.

Additionally, switching back to "no inheritance" left these items stuck in their previous checked state with no way
to manually correct it since they are always disabled. Everything is displayed consistently again only after reloading the page.

This is happening on chrome and firefox - I made you a screen-record.

The changes in my review fixes this inconsistency.

Even though it makes perfect sense to me to mark these types (like alias, url etc.) as well in conjunction with the new "Inherit" option. I still find it confusing that they are marked in the exact same way, given that it is quite obviously impossible to load a module onto them (since there is, after all, no actual page to load a module for these types).

Instead of this...

I personally would prefer something like this

I opened a PR on your Repo for this additional style changes,

TLWebdesign#1

Now you can decide, if you only want the js fix from the review or both :)

Of course i want improvements. Thanks very much 😁

Only one questiion - if I move a menu item out of subtree that was inherit

than it is expected behaviour that it stays checked, right?

Yes things should stay checked.

I have tested this item ✅ successfully on 0457056

I have tested this item ✅ successfully on 0457056

I have tested this item ✅ successfully on 0457056

Hi @TLWebdesign, I was able to test this again - successfully (and the grey-out checkbox on Safari for Heading, URL etc are better).

I get the use of the feature, and I like it... the only thing visually that makes it a bit hard on my brain is the placement of the ∨ (caret symbol) between the ∟ and the ✅ on the line, it might be easier to follow visually if the order was:

∟
∨ ∟ ✅
∨ ∟ ⃞

instead of:

∟
∟ ∨ ✅
∟ ∨ ⃞

I have tested this item ✅ successfully on 0457056

Hi @TLWebdesign, I was able to test this again - successfully (and the grey-out checkbox on Safari for Heading, URL etc are better).

I get the use of the feature, and I like it... the only thing visually that makes it a bit hard on my brain is the placement of the ∨ (caret symbol) between the ∟ and the ✅ on the line, it might be easier to follow visually if the order was:

∟
∨ ∟ ✅
∨ ∟ ⃞

instead of:

∟
∟ ∨ ✅
∟ ∨ ⃞

I get the use of the feature, and I like it... the only thing visually that makes it a bit hard on my brain is the placement of the ∨ (caret symbol) between the ∟ and the ✅ on the line, it might be easier to follow visually if the order was:

That would be separate PR and out of scope of this one. The caret before the content makes sense to me tho because you are expanding the thing that comes after it. instead of what is "around" it if you adjust it like you proposed.

I've restored the previous human test results in the issue tracker as the 2 commits which have invalidated the test count were not related to the tests:

• The update SQL scripts where changed to use the /** CAN FAIL **/ installer hint. That has been suggested by me and I have verified that it has been applied correctly in this PR.
• Code comments were improved.

RTC

RTC

sorry but I find this really confusing especially understanding the difference between "inherit" and "inherit all"

when i see inherit next to a menu item i assume it means that this menu item inherits its module assignments but it actually means something else "the modules for this menu item are inherited by the child menu items"

sorry but I find this really confusing especially understanding the difference between "inherit" and "inherit all"

when i see inherit next to a menu item i assume it means that this menu item inherits its module assignments but it actually means something else "the modules for this menu item are inherited by the child menu items" Except when its "inherited from ancesstor" when it does mean both "the menu item inherits from the parent" AND "the child menu items inherit from this setting"

I get the use of the feature, and I like it... the only thing visually that makes it a bit hard on my brain is the placement of the ∨ (caret symbol) between the ∟ and the ✅ on the line, it might be easier to follow visually if the order was:

100% agree with you. Even though I knew there was an open issue for this (#44764) and it was something I had even tried to fix I was still tricked by it with this PR wondering why it was inheriting by sample layouts and nto blog

i can see that this works but I find it very confusing

i can see that this works but I find it very confusing

Do you have any idea how to improve?

The first is to fix the existing bug with the way items at the same level do not appear to be when one or more also have sublevels

The other is finding different terminology as inherit is the wrong word but I don't have a suggestion yet. (Children inherit form parents but in this UI it's the parents that have the label inherit). Propogate might be a more accurate word but it's not very understandable

i wonder if it makes more sense in english to say inheritable instead of inherit

i wonder if it makes more sense in english to say inheritable instead of inherit

OK so i thought about it and discussed it with codex to be fair. We came up with these two options:
If we want to stay consistent in our naming in feature and in UI i would go for inherit. if we don't think it matters much if we call the feature "module menu inhertance" but n UI we speak about assignments, then the second option would also be an option. I prefer first.

First option

COM_MODULES_ENABLE_INHERITANCE_LABEL="Enable Module Assignment Inheritance"
COM_MODULES_ENABLE_INHERITANCE_DESC="When enabled, child menu items can inherit module assignments from parent menu items on the assignment tab."

COM_MODULES_INHERIT_NONE="No inheritance"
COM_MODULES_INHERIT="Child items inherit"
COM_MODULES_INHERIT_ALL="All descendants inherit"
COM_MODULES_INHERITANCE_MENU_ITEM_LABEL="Inheritance setting for %s"
COM_MODULES_INHERITED_FROM_ANCESTOR="Inherits from ancestor"

Second option

COM_MODULES_ENABLE_INHERITANCE_LABEL="Enable Automatic Child Assignment"
COM_MODULES_ENABLE_INHERITANCE_DESC="When enabled, modules can be automatically assigned to child menu items from the menu assignment tab."

COM_MODULES_INHERIT_NONE="No child assignment"
COM_MODULES_INHERIT="Assign to child items"
COM_MODULES_INHERIT_ALL="Assign to all descendant items"
COM_MODULES_INHERITANCE_MENU_ITEM_LABEL="Child assignment setting for %s"
COM_MODULES_INHERITED_FROM_ANCESTOR="Assigned from ancestor"

OK so i thought about it and discussed it with codex to be fair. We came up with these two options: If we want to stay consistent in our naming in feature and in UI i would go for inherit. if we don't think it matters much if we call the feature "module menu inhertance" but n UI we speak about assignments, then the second option would also be an option. I prefer first.

First option

COM_MODULES_ENABLE_INHERITANCE_LABEL="Enable Module Assignment Inheritance"
COM_MODULES_ENABLE_INHERITANCE_DESC="When enabled, child menu items can inherit module assignments from parent menu items on the assignment tab."

COM_MODULES_INHERIT_NONE="No inheritance"
COM_MODULES_INHERIT="Child items inherit"
COM_MODULES_INHERIT_ALL="All descendants inherit"
COM_MODULES_INHERITANCE_MENU_ITEM_LABEL="Inheritance setting for %s"
COM_MODULES_INHERITED_FROM_ANCESTOR="Inherits from ancestor"

Second option

COM_MODULES_ENABLE_INHERITANCE_LABEL="Enable Automatic Child Assignment"
COM_MODULES_ENABLE_INHERITANCE_DESC="When enabled, modules can be automatically assigned to child menu items from the menu assignment tab."

COM_MODULES_INHERIT_NONE="No child assignment"
COM_MODULES_INHERIT="Assign to child items"
COM_MODULES_INHERIT_ALL="Assign to all descendant items"
COM_MODULES_INHERITANCE_MENU_ITEM_LABEL="Child assignment setting for %s"
COM_MODULES_INHERITED_FROM_ANCESTOR="Assigned from ancestor"

I prefer the 2nd option... I think using the turn 'child assignment' goes more inline with parent vs child menu items.

I need an opinion from a native English speaker here. Technically it works — we just need to agree on the wording. I tend to prefer the second option.

@MacJoom we had a meeting about this last week. There should be a lot of changes coming - not just the strings - as other issues were identified with the UI

@MacJoom we had a meeting about this last week. There should be a lot of changes coming - not just the strings - as other issues were identified with the UI

Thanks i have it on 'Do not merge'