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

New faster navigation with dropdown menus for Plone #2516

Closed
agitator opened this issue Sep 14, 2018 · 26 comments
Closed

New faster navigation with dropdown menus for Plone #2516

agitator opened this issue Sep 14, 2018 · 26 comments

Comments

@agitator
Copy link
Member

@agitator agitator commented Sep 14, 2018

Responsible Persons

Proposer: Peter Holzer (@agitator)

Seconder: Johannes Raggam (@thet)

Abstract

Plone sites typically have a lot of content, the global navigation only supports the main folders of the site for navigation. It's time we have dropdown menus in Plone by default.

Motivation

webcouturier.dropdownmenu is used by many plonistas to solve this problem - not without issues.

  • It doesn't follow the mobile first paradigm, which was introduced with the plonetheme.barceloneta in Plone 5
  • Performance issues: takes several second on a cold Plone instance to render because it uses multiple catalog queries to build the markup

Assumptions

  • It's time we have dropdown menus in Plone by default

Proposal & Implementation

While I was in process of updating the styles for mobile first, Johannes found out about the performance issues while profiling a client site. We then decided to start a new implementation of a dropdown navigation for Plone.

The new implementation features:

  • Mobile first styling
  • Extends the existing navigation styles and ui switches from plonetheme.barceloneta
  • Build-in efficient caching
  • exclude_from_nav indexer
  • Renders on the navigation root object and sets inPath and current CSS classes in JavaScript, which allows efficient caching.
  • Replacement for the plone.global_sections viewlet
  • plone.tiles based navigation
  • Focus on speed of execution
  • Configurable navigation start path
  • Configurable navigation depth

Todo:

  • Finish styling
  • JSON data structure for client side rendering.
  • Finish caching mechanism
  • Extend existing Plone navigation controlpanel with options for navigation depth (start), caching
  • Upgrade step

Deliverables

Existing code and proof of concept in https://github.com/collective/collective.navigation (still needs polishing)

Risks

  • Existing customizations of the plone.global_sections viewlet have to be updated

Participants

  • Peter Holzer - UI/UX, frontend, styling,...
  • Johannes Raggam - Development, backend

https://github.com/plone/buildout.coredev/blob/5.2/plips/plip-2516-navigation.cfg

Tasks necessary for the plip:

  • add NavTreeProvider as contentprovider
    • enhance NavTreeProvider with globalsections functionality
  • add BooleanIndex exclude_from_nav
  • add Upgrade step for index
  • add Navigationmarker pattern
  • add Styles for new navigation
  • Extend navigation controlpanel and use config in NavTreeProvider
  • Finish styling
    • current items
    • submenu marker
    • keep current path open on mobile
  • Accessibility Markup (aria)
  • Caching (has no impact)
  • Tests
  • Update documentation, changelogs, ...

Nice to have (could be finished later):

  • Tile
  • JSON
@pbauer pbauer added this to the Plone 5.2 milestone Sep 14, 2018
@pbauer pbauer added this to New (drafts) in PLIPs Sep 14, 2018
@agitator
Copy link
Member Author

@agitator agitator commented Sep 14, 2018

@agitator
Copy link
Member Author

@agitator agitator commented Sep 14, 2018

Where should the code go?

  • Viewlet & Tile into plone.app.layout or Products.CMFPlone?
  • Styles plonetheme.barceloneta
  • Javascript Products.CMFPlone (mockup)?
@hvelarde
Copy link
Member

@hvelarde hvelarde commented Sep 14, 2018

the tile must live out of core for obvious reasons.

@jensens
Copy link
Member

@jensens jensens commented Sep 14, 2018

@hvelarde no, it can be included with a zcml:condition. Thats fine .

@frapell
Copy link
Member

@frapell frapell commented Sep 14, 2018

@agitator I think viewlet & tile in p.a.layout, and JS in mockup, if it is a pattern

@iham iham self-assigned this Sep 18, 2018
@iham
Copy link
Member

@iham iham commented Sep 18, 2018

"It's time we have dropdown menus in Plone by default"
i can sooo agree to that!

small, but nice typo:
"Finish cashing mechanism"
i guess it's about caching, not cashing ;)

@agitator
Copy link
Member Author

@agitator agitator commented Sep 18, 2018

fixed the cashing ;-)

@iham
Copy link
Member

@iham iham commented Sep 18, 2018

+1 for
viewlet in plone.app.layout -> sections.pt was there
js in mockup -> patterns/mockups live there
css in barceloneta. -> anything else is styled there.

i can't say anything about the tile, as i'm not really into the tile/mosaic topic

@agitator
Copy link
Member Author

@agitator agitator commented Sep 18, 2018

after discussion with @jensens I'll do the base navigation markup as a content provider and put the tile into plone.app.standardtiles

@jensens jensens added this to To do in Saltlab Sprint Sep 27, 2018
@agitator agitator self-assigned this Oct 1, 2018
@iham iham removed their assignment Oct 1, 2018
@pbauer pbauer moved this from To do to In progress in Saltlab Sprint Oct 2, 2018
@polyester
Copy link
Member

@polyester polyester commented Oct 4, 2018

@agitator in following up to the chat we had earlier, some links that might help:

@jensens jensens moved this from New (drafts) to Submitted (info complete) in PLIPs Oct 9, 2018
@yurj
Copy link

@yurj yurj commented Oct 10, 2018

Please consider compatibility with other navigations like collective.editablemenu or megamenu

@agitator
Copy link
Member Author

@agitator agitator commented Oct 10, 2018

@yurj please explain, imho those add ons follow a totally different approach to build a navigation

From previous discussion, what I think a mega menu should be https://community.plone.org/t/site-map-and-mega-menu-generation-is-slow/4662/17?u=agitator

@ebrehault
Copy link
Member

@ebrehault ebrehault commented Oct 23, 2018

@agitator your PLIP has been approved by the Framework team.

@jensens jensens moved this from Submitted (info complete) to In Process (approved) in PLIPs Oct 23, 2018
@agitator
Copy link
Member Author

@agitator agitator commented Oct 23, 2018

😎 ... work will continue at the ploneconf sprint

@agitator
Copy link
Member Author

@agitator agitator commented Nov 7, 2018

@thet could you look into

  • add portal_actions functionality
@thet
Copy link
Member

@thet thet commented Nov 7, 2018

@agitator I try to get this done this week.

@agitator
Copy link
Member Author

@agitator agitator commented Nov 7, 2018

@thet i also added navigation_depth
and some tests would be useful ;-)

@thet
Copy link
Member

@thet thet commented Nov 7, 2018

I'm incredibly busy currently and I hope for some unexpected productivity boosts to get anything done this week. So I don't make any promises.
Maybe this could be added to the sprint's topics.

@agitator
Copy link
Member Author

@agitator agitator commented Dec 31, 2018

As far as I see the plip is ready to merge for the alpha. Functionalitywise everthing is there, further improvements will follow on feedback.

@jensens jensens moved this from In Process (approved) to Stalled (approved but inactive) in PLIPs Jan 15, 2019
@agitator
Copy link
Member Author

@agitator agitator commented Jan 15, 2019

@svx @polyester is there documentation for the navigation somewhere to add this feature?

@polyester
Copy link
Member

@polyester polyester commented Jan 15, 2019

@agitator it depends on what the audience is for the documentation. I'm guessing here it would be for site administrators with perhaps a few hints for themers. In that case, I would put create a new rst file under https://github.com/plone/documentation/tree/5.1/adapt-and-extend/config

If there's considerable documentation for themers because it's very complicated to theme, it might warrant an own section under https://github.com/plone/documentation/tree/5.1/adapt-and-extend/theming, but if it's fairly minimal and basically just says "here's the classes, go write css if you want" it is probably easier to keep everything together.

Documentation for developers on how to actually extend/improve this dropdown-navigation should live apart from that; if this is not an own product (as I gather it's going into several other products), I would say it's easiest to create an rst file under https://github.com/plone/documentation/tree/5.1/develop/plone/functionality, that seems the most logical place

@agitator
Copy link
Member Author

@agitator agitator commented Jan 15, 2019

@polyester thanx for the pointers! I think extending the config docs should be enough.

@jensens jensens moved this from Stalled (approved but inactive) to Under Review (implemented) in PLIPs Jan 22, 2019
@fredvd
Copy link
Member

@fredvd fredvd commented Jan 30, 2019

@agitator Is there a public Demo site running somewhere? If not I should build a local coredev 5.2 with this plip activated to try the new navigation?

@ale-rt
Copy link
Member

@ale-rt ale-rt commented Feb 6, 2019

This PLIP was completed reviewed and merged in the Alpine city sprint.
Thanks you all for the hard work!

@ale-rt ale-rt closed this Feb 6, 2019
Saltlab Sprint automation moved this from In progress to Done Feb 6, 2019
ale-rt added a commit to plone/plone.app.layout that referenced this issue Feb 6, 2019
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 7, 2019
Branch: refs/heads/master
Date: 2019-02-06T20:23:16+01:00
Author: ale-rt (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.app.layout@b13a0fd

Remove a property that was defined but not used

Refs. plone/Products.CMFPlone#2516

Files changed:
M plone/app/layout/navigation/navtree.py
Repository: plone.app.layout

Branch: refs/heads/master
Date: 2019-02-07T12:15:03+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.layout@64dc4e8

Merge pull request #178 from plone/2516-fixup

Remove a property that was defined but not used

Files changed:
M plone/app/layout/navigation/navtree.py
ale-rt added a commit that referenced this issue Feb 9, 2019
Refs. #2516
ale-rt added a commit to plone/plone.app.layout that referenced this issue Feb 9, 2019
@jensens jensens moved this from Under Review (implemented) to Merged in PLIPs Feb 10, 2019
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 19, 2019
Branch: refs/heads/master
Date: 2019-02-09T16:11:46+01:00
Author: ale-rt (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.app.layout@bcc4fb5

Improve the new navigation and add test coverage

Refs. plone/Products.CMFPlone#2516

Files changed:
A news/181.fixed
A plone/app/layout/navigation/tests/test_navtree_provider.py
M plone/app/layout/navigation/navtree.py
M plone/app/layout/testing.py
Repository: plone.app.layout

Branch: refs/heads/master
Date: 2019-02-18T18:28:28+01:00
Author: ale-rt (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.app.layout@544b37b

Merge remote-tracking branch 'origin/master' into 2516-fixup

Files changed:
M CHANGES.rst
M setup.py
D news/176.bugfix
D news/180.bugfix
D news/2516.feature
Repository: plone.app.layout

Branch: refs/heads/master
Date: 2019-02-18T18:52:12+01:00
Author: ale-rt (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.app.layout@ea2cacc

Old tab could also have a name

Files changed:
M plone/app/layout/navigation/navtree.py
Repository: plone.app.layout

Branch: refs/heads/master
Date: 2019-02-18T23:53:10+01:00
Author: ale-rt (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.app.layout@c95e85e

Update and expand tests

Files changed:
M plone/app/layout/viewlets/tests/test_common.py
Repository: plone.app.layout

Branch: refs/heads/master
Date: 2019-02-19T09:32:10+01:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.app.layout@a1993c9

Merge pull request #182 from plone/2516-fixup

Improve the new navigation and add test coverage

Files changed:
A news/181.fixed
A plone/app/layout/navigation/tests/test_navtree_provider.py
M plone/app/layout/navigation/navtree.py
M plone/app/layout/testing.py
M plone/app/layout/viewlets/tests/test_common.py
ale-rt added a commit to plone/plone.app.layout that referenced this issue Mar 4, 2019
Remove the ``plone.navtree`` content provider that was introduced in the context of the Navigation PLIP (plone/Products.CMFPlone#2516)

Fixes #188
ale-rt added a commit to plone/plone.app.layout that referenced this issue Mar 4, 2019
Remove the ``plone.navtree`` content provider that was introduced in the context of the Navigation PLIP (plone/Products.CMFPlone#2516)
The logic has been moved to the viewlet that was invoking it.

Fixes #188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PLIPs
Merged
Linked pull requests

Successfully merging a pull request may close this issue.

None yet