Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

menu system does not allow some menu strategies for touch devices #3527

Closed
jrief opened this issue Oct 21, 2014 · 43 comments
Closed

menu system does not allow some menu strategies for touch devices #3527

jrief opened this issue Oct 21, 2014 · 43 comments

Comments

@jrief
Copy link
Contributor

jrief commented Oct 21, 2014

Bootstrap 3 introduced a navigation bar, where on mouse over, the sub/pulldown-menu does not automatically pop up. This brought to some controversial discussions on the Bootstrap mailing list, but considering the importance of touch devices (no mouse over possible), I agree with dropping support for this.

The menu system in django-cms is built in a way, which in practice requires the mouse over effect. The reason is, that one can not create a CMS "page", whose purpose is to only act as a node in the menu tree, without offering any page content.

For instance, consider the menu system of a typical site build with Bootstrap 3: https://www.angularjs.org/ . Here the main menu contains items: 'Learn', 'Develop', 'Discuss', which actually do not offer any page content.
Now, assume one of the above items shall additionally offer some page content. This can be achieved by adding a copy of an item from the main menu, into its own pulldown menu. Here, the first click onto the main menu's item opens the pulldown menu. Then, a second click on the copy of that item, opens the requested page.

In django-CMS we could use the flag "in menu", but when this is turned off, then all menu items below that item are unselectable, and that's definitely not want we want.

My proposal is to add another magic template. Currently django-cms has a built-in magic template "Inherit from Parent". We could add another magic template, say "Empty Page", which then tells the menu system to act as a clickable menu item, but not offering any page content. Then, while building the menu, the template tag menu can distinguish a page acting as pure navigation node from a real cms page.

If this proposal is accepted, I can implement it.

EDIT (by ojii): Note, the title of this issue was changed by me.

@yakky
Copy link
Member

yakky commented Oct 24, 2014

While workarounds exist to implement this without changes to the CMS, I agree that having a standard way to handle this would be a nice thing.
I'm not sure another "magic template" would be the best approach (I hate "magic" things, actually), even if it has the least impact on the rest of the structure.
@jrief any other sensible option?
Ping @mkoistinen @evildmp @digi604 @ojii for further input

@yakky yakky added this to the 3.1 milestone Oct 24, 2014
@ojii
Copy link
Contributor

ojii commented Oct 24, 2014

I don't like using the template for this. Right now you could actually emulate this behaviour by making a page with a redirect to it's first child on it, so clicking it actually does something which is even better! How about a redirect to first child flag (a new field) to make this easier should there really be a big need for it?

@yakky
Copy link
Member

yakky commented Oct 24, 2014

Actually you can't use redirect as you'll want the click to just open the dropdown on touch devices.
I was thinking more of a flag empty page or non-leaf page passed as an attribute to the relevant navigation nodes

@mkoistinen
Copy link
Contributor

Firstly: I think the title of this issue is rather alarmist and very mis-leading. I've deployed many touch-capable sites with CMS. The BS3 method of dealing with touch devices isn't the "correct" way, but a way and CMS has no particular affiliation with the BS3 project (other than knowing it is awesome =)

Secondly, I do agree that the current menu system is somewhat limited and is often frustrating because we cannot make intermediate organisations that aren't also pages in their own right. In these cases, the CMS is the frustrating part.

Sometimes (most of the time?) though, having a proper-page that is a "parent" for its subpages is a good thing and it is here where BS3 fails the CMS developer. For example, here's a recent way I've had to "bastardize" BS3 to make a drop down that also works as a direct link the Products page.

image

In the case, you can click the words "Products" or you can click the caret to revel the pages beneath.

@mkoistinen
Copy link
Contributor

+1000 @yakky's 'empty_page' flag.

@ojii
Copy link
Contributor

ojii commented Oct 24, 2014

"empty page" is easier to understand than "non-leaf node", but maybe "menu node" or something like that would be even clearer (bikeshedding, yay!)

@yakky
Copy link
Member

yakky commented Oct 24, 2014

I'm so bad at naming, that it's granted that my proposals are the most horrible anyone can come up with.
"menu node" looks quite good to me

@mkoistinen
Copy link
Contributor

+1 "menu node"

@ojii ojii changed the title menu system not suitable for touch devices menu system does not allow some menu strategies for touch devices Oct 24, 2014
@jrief
Copy link
Contributor Author

jrief commented Oct 24, 2014

Adding another field to the node was an alternative I also considered.
The reason to use a magic template was, because in my opinion it makes sense to have a CMS page with intentionally no content. Therefore, in my opinion, such a template isn't that magic.
But of course, if the core developers have another opinion, I have no problem. The important thing is, that we can have empty pages acting exclusively as nodes.

@yakky
Copy link
Member

yakky commented Oct 24, 2014

@jrief go for it, thanks! Also I'd like to have this in 3.0.x branch, if the patch will not be too intrusive

@ojii
Copy link
Contributor

ojii commented Oct 24, 2014

@yakky why 3.0.x? This is a new feature, so should go to 3.1, right?

@jrief
Copy link
Contributor Author

jrief commented Oct 24, 2014

Just to be clear, so we'll go with an extra boolean field Page.menu_node = BooleanField(...) rather than a magic template.
The checkbox for this field will be available in the advanced section of the model admin.

@yakky
Copy link
Member

yakky commented Oct 24, 2014

@ojii as we just started the 3.1 development cycle, IMHO we may still have room for integrating minor but useful features into next 3.0 release (just an opinion, though)
@jrief 👍

@ojii
Copy link
Contributor

ojii commented Oct 25, 2014

@yakky IMHO once you start distinguishing between "minor" and "major" features the whole distinction between "x.Y.z" and "x.y.Z" releases becomes moot. I think the following system is quite logical and good:

x.y.z
^ ^ ^
| | |- Bug Fixes
| |- Features
|- CHANGE EVERYTHING

EDIT: If the problem is that this means it'll take a long time for this feature to be available, maybe we should try (like every release ;-) to make 3.1 happen in a reasonable timeframe.

@mkoistinen
Copy link
Contributor

We should do this in the 3.1 branch, mainly because this is related to the menus. The primary distinction (so far) on 3.1 is that we've (by "we" I mean "Patrick" =) ripped out mptt and replaced it with treebeard. As Jacob (or whomever) develops this feature, he'll likely be touching some of this and I'd rather we didn't have to support two underlying tree systems and can instead optimize for one.

Also, as we go forward with this change, we're likely to be further testing Patrick's treebeard replacement even further, and possibly spot some otherwise difficult to find bugs. I realize that the treebeard is at a lower abstraction layer, but we're bound to touch on it in a material way during the implementation of this feature. And if we don't, then that also tests that our abstraction layer is properly abstracted =)

Also, 3.0.x is now labeled "Support 3.0.x"... we don't add new features directly into a support branch. This may creep in there as a back-port though.

@ojii. I agree 100% with your major.minor.bug-fix system! In fact, if we stuck to it, then the mptt->treebread change would have been on a x.x.X branch, and this extra menu feature would be a x.X.0 release, because features are user-facing. In my book, if it doesn't affect a user in a visible way, then it's not a feature. But c’est la vie.

tl;dr; this should go in the 3.1 branch.

@ojii
Copy link
Contributor

ojii commented Oct 25, 2014

@mkoistinen changing the fundamental data model of a project isn't quite a "bugfix" though. arguably it should be 4.0 if anything.

@mkoistinen
Copy link
Contributor

Oh and thanks for changing the issue title, @jacob =)

@ojii
Copy link
Contributor

ojii commented Oct 25, 2014

That was actually me @mkoistinen (should've written that in a comment, sorry)

@mkoistinen
Copy link
Contributor

+1

@Noah-A
Copy link

Noah-A commented Oct 25, 2014

It would be exceedingly helpful if I could add Pages in multiple locations in the Menu in such a way as to have translation sensitivity. So for example I may have Page called "Processes" for which I could have a menu item somewhere else, (for example it's first child) possibly labeled as "Process Overview" which simply takes the user to the "Processes" Page but which also works when the Page has a different slug and name in different languages.

@ojii
Copy link
Contributor

ojii commented Oct 26, 2014

@Noah-A how is this related to this issue? It sounds like a Page with a redirect on it to me.

@yakky
Copy link
Member

yakky commented Oct 26, 2014

@ojii @mkoistinen +1

@mkoistinen
Copy link
Contributor

@ojii I asked @Noah-A to put this issue here. In retrospect it should probably be its own issue. The reason I asked him to note it here is that I think that there are larger issues with menus that should be addressed with a larger plan of attack and not a piece-meal approach. I will create a new issue from Noah's points. The issue is larger than what it seems.

Created new issue: #3535
See related discussion topic started here: https://groups.google.com/forum/#!topic/django-cms-developers/4DysrNgkBL4

@jsma
Copy link
Contributor

jsma commented Oct 27, 2014

The empty page concept seems like it could/would solve a related issue: the pages grouped under "Learn", "Develop", "Discuss" in the navigation might not need or in some cases shouldn't have '/learn/', '/develop/' as a component of the url.

For example, perhaps the site was originally designed with an "About Acme" page at the root (e.g. /about-acme/) but for navigation purposes, this is now grouped under 'Learn'. Cool urls don't change yadda yadda so if this 'Learn' was an empty page and did not contribute any slug bits to the pages beneath them in the tree, then 'About Acme' could be moved in the tree under 'Learn' so it appears in the right spot in the menu but without any 301 redirects or other customizations required.

My current project has the exact use case the OP mentions but I could not wrap my head around how to get the menu system to work smoothly without adding '/learn/' as a prefix to every page, especially when there really isn't a 'Learn' page, it's just an artifact of the menu organization/UI and shouldn't add any cruft to the urls.

@jsma
Copy link
Contributor

jsma commented Oct 27, 2014

Ugh, I'm having a moron Monday (month more accurately). Just now realizing/remembering there is "Overwrite URL". Apologies for the noise and +1 to a built-in solution for non-clickable top-level menu items.

@evildmp
Copy link
Contributor

evildmp commented Nov 3, 2014

I am not keen on adding a new page flag for empty_page. It's arbitrary and has no real connection with Pages, only with the menus. Even in_navigation is a bit poor: which navigation? We might have two different menus.

What I think we need is a scheme for arbitrary page attributes. Then we could have whatever flags we wanted:

  • in_main_menu
  • in_special_menu
  • menu_node_only (for empty pages whose only purpose is to create a node in the menu)
  • christmas (load the special christmas.css)
  • display_secret_menu

Templates would have access to these attributes, and could act on them as appropriate.

For example:
{% if current_page.flags. display_secret_menu %}...{% endif %}

For menu templates to act on these attributes we'd need to do make some revisions to the menu code, so that menu nodes have a generic FK to the objects they represent (which I think they should have anyway).

https://github.com/evildmp/django-cms/compare/pageflags-patch does an excellent job of adding arbitrary page flags.

Whatever we do, I would prefer to be cautious and not just solve the immediate problem with a quick fix that simply bolts some new functionality on.

@ojii
Copy link
Contributor

ojii commented Nov 3, 2014

What I think we need is a scheme for arbitrary page attributes. Then we could have whatever flags we wanted:

Isn't this what page extensions [1] do?

[1] http://docs.django-cms.org/en/latest/extending_cms/extending_page_title.html

@jrief
Copy link
Contributor Author

jrief commented Nov 3, 2014

During my first attempt to implement this, I came across another pagemodel field: limit_visibility_in_menu, whose purpose is not clear to me. I was not even able to change that value through the admin interface.
This field states: "limit when this page is visible in the menu"
Q1: How can I activate this?
Q2: Does it make sense to extend this field to be used for the proposed feature?

@evildmp
Copy link
Contributor

evildmp commented Nov 3, 2014

@ojii yes of course, my brain is still on 2.3.5.

@evildmp
Copy link
Contributor

evildmp commented Nov 3, 2014

@jrief I think this is to do with pages that require authentication: should they be displayed in menus?

@ojii
Copy link
Contributor

ojii commented Nov 3, 2014

@evildmp welcome to 3.0 :D

However this raises the question, does this actually require a change in core if it could be solved with a 3rd party cms page/title extension? I've never used this new feature so I'm not 100% sure on the implications, but this got added for a reason, so why not use it here?

@evildmp
Copy link
Contributor

evildmp commented Nov 3, 2014

I think it would still require changes to the menu system - a way of getting from a menu node to a Page in the template.

@ojii
Copy link
Contributor

ojii commented Nov 3, 2014

This'll be a bit more complicated than that, since not all menu nodes have a Page associated. But maybe a node.object attribute that may point to a instance of a model? (And maybe node.object_type which is a string in the form of app_modelname to check the type in the template)

@evildmp
Copy link
Contributor

evildmp commented Nov 3, 2014

A generic FK to the object.

@yakky
Copy link
Member

yakky commented Nov 3, 2014

Page extension requires an additional join which is not always good performance wise.
I think this may worth a standard solution for a common class of problems.
Be it an official page extension or something in core it's up for discussion

@mkoistinen
Copy link
Contributor

I think we all need to realise that this and other issues stem from the fact that the Page model is being overloaded with two distinct concepts: "Page" and "Menu Node". They're not the same thing. We should be looking for a way to de-couple them.

@ojii
Copy link
Contributor

ojii commented Nov 3, 2014

A generic FK to the object.

Menu nodes aren't database objects, so no (G)FKs here.

Page extension requires an additional join which is not always good performance wise.
I think this may worth a standard solution for a common class of problems.
Be it an official page extension or something in core it's up for discussion

I'd say either do it in core proper, or a separate 3rd party package.

I think we all need to realise that this and other issues stem from the fact that the Page model is being overloaded with two distinct concepts: "Page" and "Menu Node". They're not the same thing. We should be looking for a way to de-couple them.

That's not 100% true. The default cms menu [1] turns most pages into menu nodes. I think this makes a lot of sense, since you usually want your cms pages to be navigateable.

[1] https://github.com/divio/django-cms/blob/develop/cms/menu.py#L220

@yakky
Copy link
Member

yakky commented Nov 3, 2014

@mkoistinen actually pages are the only user-changeable materialization of a menu node in django CMS, so it's pretty natural to handle the relevant menu options at the Page model level

@jrief
Copy link
Contributor Author

jrief commented Nov 3, 2014

When I looked at the code I had the same feeling as @mkoistinen , the menu/node system shall be decoupled from the pages. I remember a few posting from people using djangoCMS saying, that they only use the Page-model without menu system (or vice versa, I don't remember). Anyway, it means that there is a "market" for separating these the page from the node.
For instance, djangoSHOP could perfectly use the menu system to build categories. Or one might want to build a URL resolver the "Django-way", but adding pages editable in the front-end. Therefore that idea is not unreasonable.

I have to ask again:
Q1: How can I activate limit_visibility_in_menu?
Q2: Does it make sense to extend this field to be used for the proposed feature?

@mkoistinen
Copy link
Contributor

image

@ojii, here's how I see it for most projects and this doesn't even include the menus I end up hard-coding into the markup because I see no other way to properly add them ("Login" and "Logout" are regular examples). I only have one thing on the right: "App menus", but in reality, this is a pretty large collection of things in most projects.

@yakky, I completely agree, but herein lies the issue. The concept and implementation of a Page is tantalizingly close to what we need for a menu item, so, we run with it. And, its not just the model, its also the slick GUI (page tree), which would be a large undertaking to make something new just for menus.

But, the fact is, they are not the same thing.

I've used Drupal and Wordpress before Django CMS. Now, I'll state right here and now my preference is for Django CMS by a wide margin, but in both of those systems, the concept of a menu is entirely decoupled from their version of a page. In fact, in both of those systems, they have multiple "things" that can be added to menus, not just pages. We have app-hooks, which serve the purpose of those other "things", and we can attach them to menus via app-menus in a limited manner (must be a child of a page to which they are attached, and, we can only attach one set of menus to that page), but in the other systems, you can also:

  1. Define multiple independent menus - using the show_menu template tag we can slice up the menu into pieces, but they're not independent from one another. For example, if two 'slices' have some of the same items, you cannot re-order those items independently in the respective slices;
  2. Attach an item multiple times to multiple menus if you wish - we cannot do this in an operator friendly way today (this very issue is case-in-point, but there are several other use-cases);
  3. You can arbitrarily change the order of the items (see Cross Domain Request Error  #1 above);
  4. You can attach simple URLs anywhere - we do this with a hack. And, it has issues. For instance, because it is implemented by overloading the Page model, it is not possible to have language-dependent URL destinations. This should have been implemented by overloading the Title model.

I'm not necessarily advocating that we just rip out what we have and replace it all. I think it would be very possible to fix some of these shortcomings. For No. 4, we can probably add I18N to redirects by leveraging the Title model as it was intended. For No. 2, I can imagine a non-core app that provides an operator-friendly GUI for attaching arbitrary objects (including pages and even arbitrary urls properly) as menu items into arbitrary places in the existing menu via Menu Modifiers. This may require some relatively simple changes (like allowing more than one menu attachable to pages, in an ordered manner), but shouldn't require a complete overhaul at all.

@ojii
Copy link
Contributor

ojii commented Nov 4, 2014

For instance, djangoSHOP could perfectly use the menu system to build categories.

Assuming categories are a model, can't you just use the menu extension system to add these categories? Is the main issue being able to re-arrange these nodes?

here's how I see it for most projects and this doesn't even include the menus I end up hard-coding into the markup because I see no other way to properly add them ("Login" and "Logout" are regular examples). I only have one thing on the right: "App menus", but in reality, this is a pretty large collection of things in most projects.

having login/logout hardcoded seems fine with me. I don't consider them part of the page/menu hierarchy, but rather "global" functionality.

@mkoistinen
Copy link
Contributor

having login/logout hardcoded seems fine with me. I don't consider them part of the page/menu hierarchy, but rather "global" functionality.

But they are! Our operators should be able to move those around from position to position or menu to menu without any hassle.

@jrief
Copy link
Contributor Author

jrief commented Nov 19, 2014

While implementing this feature, I came to the conclusion, that it is a bad idea to add another, somehow magic flag, to the Page model.

I now strongly believe that we should separate the menu tree from the page models. In the future, web-sites will be built with partial views, but each of them will have a separate URL. This means that we somehow need a way to drop the 1:1 relationship between menu-nodes and pages and replace it by a 1:n-relationship.

I'll write an extra proposal about this.

@jrief jrief closed this as completed Nov 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants