infrabel / themes-gnap

Standardized build to produce web themes for use with GNaP.
http://gnap.io/
BSD 3-Clause "New" or "Revised" License
3 stars 4 forks source link

Correctly display active sub menu items #135

Closed vincentsels closed 8 years ago

vincentsels commented 9 years ago

When a sub menu item was activated and its state navigated to, the sidebar just closed the sub menu again. Fixed so that the sub menu remains open, and the sub menu item is marked as active.

CumpsD commented 9 years ago

This already works as is, I will reply this evening. Your configuration needs to include the parent.

main.parent/main.parent.submenu On Jun 24, 2015 12:24 PM, "Vincent Sels" notifications@github.com wrote:

When a sub menu item was activated and its state navigated to, the sidebar just closed the sub menu again. Fixed so that the sub menu remains open,

and the sub menu item is marked as active.

You can view, comment on, or merge this pull request online at:

https://github.com/infrabel/themes-gnap/pull/135 Commit Summary

  • Correctly display active sub menu items

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/infrabel/themes-gnap/pull/135.

vincentsels commented 9 years ago

I see. Then I suppose it should be fixed in the state generator instead ? He just generates the sidebarKeys as follows:

sidebarKey: 'main.aftersales.machinepark.detail'

But then all existing projects need to get updated too, whereas with this solution they don't. But on the other hand, perhaps in some existing projects the developers already manually switched to ´main.parent/main.parent.sub´... hmm... :)

CumpsD commented 9 years ago

Yes, the generator is incorrect, and doesn't support nested states. Same with the url suggestion. There is a branch which started v2 of the generator, but isn't finished :(

David Cumps IT Consultant Cumps Consulting BVBA On Jun 24, 2015 12:54 PM, "Vincent Sels" notifications@github.com wrote:

I see. Then I suppose it should be fixed in the state generator instead ? He just generates the sidebarKeys as follows:

sidebarKey: 'main.aftersales.machinepark.detail'

But then all existing projects need to get updated too, whereas with this solution they don't. But on the other hand, perhaps in some existing projects the developers already manually switched to ´main.parent/main.parent.sub´... hmm... :)

— Reply to this email directly or view it on GitHub https://github.com/infrabel/themes-gnap/pull/135#issuecomment-114827070.

vincentsels commented 9 years ago

Is there a reason for explicitly having to repeat the parent states every time, when it's already always included in the dot-separated name of the state itself ? If not, would you not agree that my suggested alternative is cleaner (shorter sidebarkeys without repetition) ?

Also, in the original implementation, when selecting a sub-state of a sub-menu item which does not by itself have a menu entry (as will very often be the case, because typical 'leaf-level states' such as 'search', 'result', 'detail',... will generally never have menu entries), the sub-menu item does not seem to get 'highlighted'. Not sure if this is intended, or a bug, or if I'm still doing something wrong.

Example: in states main.parent.sub.search, main.parent.sub.result,... where 'parent' is a top-level menu with 'sub' as an entry under it, the 'parent' menu will be expanded so that the 'sub' is visible, but 'sub' is not highlighted.

CumpsD commented 9 years ago

A real reason? No. Right now they are simply developed as unrelated things. State names and sidebar keys are not related. You can have a sidebar with key X pointing to a state with a totally different name. The bug is the generator not generating the right thing.

I think your case is actually more like this, your leaf level state might be named bla.bla.search while the sidebar key for that thing might be parent/search?

vincentsels commented 8 years ago

As explained by @CumpsD , this is rather a problem with the generator, not with the system that selects the menu item. Hence, this change is not required.