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'

@TLWebdesign are there upcoming changes? as there are updates requested... Thank you

Back to pending

Back to pending

Back to pending

Back to pending

Yeah good idea. I need a couple days to get this going again. Hope to have it done soon. I wonMt do any specific promised anymore it seems i can't keep em.

@MacJoom the updates that were requested and discussed have been implemented now.

all looks good to me

I have tested this item ✅ successfully on 07bee78

Stepped through Testing instruction 1-18
Anyone for front end ?

I have tested this item ✅ successfully on 07bee78

Stepped through Testing instruction 1-18
Anyone for front end ?

@TLWebdesign - I assume there were some changes that were not updated in the Testing Instructions as I can not find Inherit and Inherit All - I'm seeing Inherit to: None / All Direct sub-items / All sub-items.

Can you please 🙏 update the testing instructions so that we matching in our testing what you are expecting / wanting us to test initially?

During Happy Friday PR testing July 31st - Martin, Thomas and I were trying to test it and came up with this and also found a bug most likely - Martin will write a comment with screenshots.

I do not expect "Workflows" to be marked as it is not a direct sub item

I do not expect "Workflows" to be marked as it is not a direct sub item
The display is wrong. Workflows was marked correctly.

I do not expect "Workflows" to be marked as it is not a direct sub item
The display is wrong. Workflows was marked correctly.
Fix might be: #48125

I do not expect "Workflows" to be marked as it is not a direct sub item
The display is wrong. Workflows was marked correctly.
Fix is #48125 (now tested by me...)

I do not expect "Workflows" to be marked as it is not a direct sub item
The display is wrong. Workflows was marked correctly.
Fix is #48125 (now tested by me...)

Testing instructions:
Step 6 instead of "inherit" select "Direct sub-items"
Step 8 instead of "inherit all" select "All sub-items"

Testing instructions:

Step 6 instead of "inherit" select "Direct sub-items"

Step 8 instead of "inherit all" select "All sub-items"

Thanks i updated the instructions. 👍

@MacJoom just confirming that your "bug" was just a result of the cosmetic issue that has been fixed in the other PR which made things look nexted when they were not etc

I just spent 30 minutes checking to see why I couldnt see any of the new menu assignment options. I had completely forgotten that I needed to enable the option first.

Does this really need to be an option that you enable. From what I can see (i could be mistaken) enabling this by default has no negative effect.

When you set an inheritance it checks the menu items
When you unset an inheritance it keeps the menu items checked
that all makes sense

Until you have a three level menu as the 2nd level now says interit none but the items below are checked

Inherit all sub-items

Change to inherit none

Proposed solution

To avoid confusion with someone saying why does it say inherit none but they appear to be inherited I propose to change the language string from NONE to Not Set

I have tested this item ✅ successfully on 07bee78

marking as a successful test as it does what it says. I would still like to see the changed language string and to reconsider it being off by default

I have tested this item ✅ successfully on 07bee78

marking as a successful test as it does what it says. I would still like to see the changed language string and to reconsider it being off by default

I would expect the grandchildren to be set back to the original state - or do i overlook something?

If a module is set to inherit to “All sub-items”, rows are set for grandchildren. Later changing the source to “Direct sub-items” leaves the old grandchild rows checked as baseline assignments in the UI and they get saved back as manual rows. Result: the module continues displaying on menu items that are no longer covered by inheritance.

I would expect the grandchildren to be set back to the original state - or do i overlook something?

Yes that is intended behaviour. You turned inheritance on. Then when you unset inheritance we never remove already assigned items so things just don't dissapear by themselves.

I disagree - when i remove inheritance i expect the previous state to be restored. Would you consider adding a second field that carry the previous state?

If a module is set to inherit to “All sub-items”, rows are set for grandchildren. Later changing the source to “Direct sub-items” leaves the old grandchild rows checked as baseline assignments in the UI and they get saved back as manual rows. Result: the module continues displaying on menu items that are no longer covered by inheritance. I would expect the grandchildren to be set back to the original state - or do i overlook something?

Yes that is intended behaviour. You turned inheritance on. Then when you unset inheritance we never remove already assigned items so things just don't dissapear by themselves.

I disagree - when i remove inheritance i expect the previous state to be restored. Would you consider adding a second field that carry the previous state?

No that is not wanted. This has been discussed and agreed on with multiple people during development and considered the way it should be. Not planning to change that now.

Unless I misunderstood your request @MacJoom then this is already possible

I disagree - when i remove inheritance i expect the previous state to be restored. Would you consider adding a second field that carry the previous state?

If a module is set to inherit to “All sub-items”, rows are set for grandchildren. Later changing the source to “Direct sub-items” leaves the old grandchild rows checked as baseline assignments in the UI and they get saved back as manual rows. Result: the module continues displaying on menu items that are no longer covered by inheritance. I would expect the grandchildren to be set back to the original state - or do i overlook something?

Yes that is intended behaviour. You turned inheritance on. Then when you unset inheritance we never remove already assigned items so things just don't dissapear by themselves.

I disagree - when i remove inheritance i expect the previous state to be restored. Would you consider adding a second field that carry the previous state?

I don't think at this stage we can add that functionality. Since module versions is a functionality in J6, does Module versions include the 'menu selection'? If so, then yes you can already do this.

I have tested this item ✅ successfully on 07bee78

marking as a successful test as it does what it says. I would still like to see the changed language string and to reconsider it being off by default

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47570.}

Should i change that language string now? It will undo the test, right? Not sure if i should do it now or later.

I can always mark the test again, that is if you agree with the change

I still wonder if it should be enabled by default. It doesn't break anything and it would be one less Secret setting

I still wonder if it should be enabled by default. It doesn't break anything and it would be one less Secret setting

Yeah i' e been thinking about that. But i feel not having it enabled by default does make the layout a bit cleaner and probably less overwhelming for not so tech savy users. When it is enabled it does add a lot more stuff to the view. Especially if you try it out. Also having it enabled by default would make it the issue martin talked about; not unselecting after enabling inheritance a bigger issue because people who are not looking for the feature will turn it on and off out of curiousity and then their menu items are all selected. Of course a page refresh is an easy fix. I just don't want to make things more "complicated" than it needs to be. So i'm torn about it. @bembelimen @LadySolveig @rytechsites what do you think about this?

I updated the language constant to Not set. Thanks 😊

strange it didnt reset the tests

strange it didnt reset the tests

strange it didnt reset the tests

Yes, strange. I have just triggered a branch update, but that also did not reset the tests in the issue tracker.

They can be reset with the "Alter test" feature in the issue tracker.

Shall I do that?

@richard67 your call but I only knew it hadnt reset because I went to put my new success test message so probably not worth your time

For me personally, this is quite a complex feature, one that I probably wouldn’t even enable on most sites. It’s great that power users now have this option, but I agree with @TLWebdesign it can certainly be overwhelming and quickly become confusing for less experienced users. I wouldn’t enable it by default.

Seems consensus is we want it behind a setting. So i think it is ready like this?

Seems consensus is we want it behind a setting. So i think it is ready like this?

i wish we were consistent on this.

I was just on Tom’s test server checking out the current implementation. I wasn’t a super user there, though.

The view within the Menu Assignments section confused me a bit at first. It seems I now have two ways to select checkboxes: one is via the “Select/Deselect” buttons next to the menu items, and the other is via the new selection options.

At first glance, it wasn’t immediately clear to me which selection is relevant for module assignment and what function the additional selection serves. After taking a closer look, the difference becomes clearer, but I wonder if we should distinguish the two functions even more clearly for users—or perhaps even standardize them. However, I haven’t delved deeply enough into the current implementation yet to be able to make a definitive judgment.

On the other hand, we should perhaps test what happens when a module is loaded within a menu module. I’m not sure if this could have any implications. This scenario could arise, for example, if you want to load a search module in the same area where the menu is displayed.

I was just on Tom’s test server checking out the current implementation. I wasn’t a super user there, though.

The view within the Menu Assignments section confused me a bit at first. It seems I now have two ways to select checkboxes: one is via the “Select/Deselect” buttons next to the menu items, and the other is via the new selection options.

At first glance, it wasn’t immediately clear to me which selection is relevant for module assignment and what function the additional selection serves. After taking a closer look, the difference becomes clearer, but I wonder if we should distinguish the two functions even more clearly for users—or perhaps even standardize them. However, I haven’t delved deeply enough into the current implementation yet to be able to make a definitive judgment.

On the other hand, we should perhaps test what happens when a module is loaded within a menu module. I’m not sure if this could have any implications. This scenario could arise, for example, if you want to load a search module in the same area where the menu is displayed.
I just tested that, and it worked like expected. :-)

The user-friendliness is a bit of an issue, because when you select something, it’s not immediately clear what it’s for. But since you can disable the entire feature ...

Maybe it’s a helpful “expert mode.” :-)
@brianteeman , I’m sure you’ve checked the accessibility, right?

@brianteeman , I’m sure you’ve checked the accessibility, right?

as the entire page is not-accessible before this PR its not any worse with this PR

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

since this one is not going into 6.2 should we just let it be for now and fix all problems when 6.3 alpha 1 is ready? I don't feel like keeping this thing up to date for half a year. Can just fix it all when it is time for 6.3, right?

since this one is not going into 6.2 should we just let it be for now and fix all problems when 6.3 alpha 1 is ready? I don't feel like keeping this thing up to date for half a year. Can just fix it all when it is time for 6.3, right?

@TLWebdesign I don't know if it will go into 6.2 or not. For now I have allowed myself to fix the merge conflicts in the 2 base.sql files and to rename your update SQL scripts to something newer than the latest one in the branch so your PR would be ready again for 6.2-dev.

We're at beta 2 today so yeah it's not going into 6.2 We're past that stage. But thanks for fixing it i guess. Just don't waste too much time on this until it makes sense to fix things when 6.3 is there.