tormec / AcMenu

DokuWiki plugin which provides an accordion menu for namespaces and relative pages.
https://www.dokuwiki.org/plugin:acmenu
1 stars 3 forks source link

Fixes #10 #13

Open eduardomozart opened 1 year ago

eduardomozart commented 1 year ago

Not sure if you will accept this PR, because it implements some major changes into the way the plug-in works, but there's some suggestions that worth checking and can be applied even if you didn't accept this PR. If you think that those changes are too much, we can find a way to make them optional, maybe adding an option to enable those features at the DokuWiki Settings.

Here's the way the plugin is working now (as stated in https://github.com/my17560/dokuwiki-template-readthedokus/issues/32):

Animação

I'll start with changes that I recommend implementing:

  1. CSS changes: There's just two lines changed at the CSS file. It adds a cursor icon at the li icon to make it intuitive to the end user that it's clickable. Other change that I made was adding the "list-item" style for the AcMenu list, because some templates like "Read the Dokus" and "Argon Alternative" does a normalization removing all inherit styles from HTML, so your plugin didn't show the list icons in those templates. I recommend you apply those changes at your CSS file, too.

Now the harder to explain ones: I took about ~8 hours to figure out how your plugin works and how I could implement the feature I wanted (I'm not exactly a Dev and it was the first time I was working with Cookies - was funny to figure out through the "Applications" tab of Dev Tools from Chrome how you implemented the browsing feature). Basically, I'm migrating from IndexMenu to your plug-in because IndexMenu seems a lot instable to me at PHP 8.1 (as can be seen here: https://github.com/samuelet/indexmenu/pull/261).

The major change I was talking about is the way that pages and namespace are handled. Now they are together, instead of showing namespace first followed by the pages (as reported in #10). So now for the user to browse through the namespace, he/she must use the li icon, otherwise it will open the page that it contains. This behaviour is the same of simpleNavi and IndexMenu plugins.

I also edited your source code to respect the hidepages DokuWiki directive, so if an admin wanted to hide pages, it will not be shown at the AcMenu.

tormec commented 1 year ago

I've tested your PR with the default template of Dokuwiki and the following are some observations.

1. menu behaviour I've tried to create a nested page: ns1:ns2:start

The original behaviour of AcMenu is that if one clicks on each name that identifies a namespace one can fold/unfold its content while the <li> element has no purpose. With your PR only the <li> element allows toggling the content whereas the namespaces are treated as links to pages (at least this is what now you are storing in the cookies).

Eventually, I could think to introduce this behaviour so that one can hide the start page (which was a very common request) and make it accessible by clicking on the relative namespace.

2. cookies The idea to use the cookies is to keep track of all open pages with their path and to remove them from the cookies as soon as they are closed. In your PR, this behaviour is completely changed: cookies are not cleaned and they are not used to visualize where one is in the menu, except the open page.

I will not change how cookies are managed.

3. hidden pages I didn't understand why now you need to check if a page is hidden or not.

Was this a bug before?

4. template compatibility As you can see, even if your PR works well with the templates that you have tested, it doesn't mean it works under others (Bootstrap3 is another example). The reason is that this plugin relies on a strict sequence of HTML elements that form the menu. Unfortunately, this is the Achilles' heel of this plugin and, instead of creating patches for each template, I would like to try to find a way to make it independent from the template itself.

PS: next time try to split such big PR into more digestible PRs, otherwise it's difficult to understand the purpose of the amendments and which part of the code works and can be merged.