search

moodle-an-hochschulen / moodle-local_boostnavigation

Moodle plugin which tries to overcome some fixed appearance behaviours of Boost's nav drawer in a clean way
GNU General Public License v3.0
39 stars 28 forks source link

Add option to position custom nodes #28 #29

Closed jrm-unamur closed 5 years ago

jrm-unamur commented 5 years ago

Hi Alex, Here is the code. 3 files were updated locallib.php to handle the extra param for custom nodes lang strings file to add documentation about the new param version.php to install the modified lang string

Cheers Jean-Roch

jrm-unamur commented 5 years ago

Dear Alex, You can wait a little before reviewing, it is not as simple as I thought. Since all default nodes are not necessarily children of myhome node, I must go deeper in the hierarchy. Thus ... needs more work. Cheers Jean-Roch

abias commented 5 years ago

Hi Jean-Roch,

I am already reviewing your patch and have already made some necessary tweaks.

I would like to ask you to pause your work until I am done. I will push my results into a branch and you can then check your edge cases.

jrm-unamur commented 5 years ago

Ok, I had just finished to fix, but leave you the priority ;-)

abias commented 5 years ago

Allright, here you go. I have pushed my changes to https://github.com/moodleuulm/moodle-local_boostnavigation/commits/LMS-3594-beforekey. Instead of addressing the things to improve here, I just went ahead and fixed them.

Feel free to diff them with your submitted code.

I would now be curious to hear about your edge cases and to see the fixes for them. I think it will be best if you rebase your fixes onto my branch and update the pull request.

jrm-unamur commented 5 years ago

Hi again, I am not familiar with git rebasing (:() . Simply using branches and tags within our university repo has so far be enough for our work. Do you mean just changing the base here in the pull request page on Github? (that would cause conflicts) Or creating a new branch based on your LMS-3594-beforekey branch and make a new pull request? Cheers

jrm-unamur commented 5 years ago

As for the edge cases, for instance the calendar node is not a direct child of the base node (myhome) used to add custom nodes. Thus when trying to add a custom node before calendar node, you get an error since $node->add_node function only searches for the "beforekey" node within the direct children of $node. I think the solution is to search for the direct parent of beforekeynode (if beforenodekey is set) $node->find($beforenodekey, global_navigation::TYPE_UNKNOWN)->parent; and call the add_node function on that very node. I have made the changes locally but have not pushed them yet; I prefer to make another few tests before. I'm off duty next week and have some things to finalize before, so I will handle that after Easter Cheers again

abias commented 5 years ago

Hi Jean-Roch,

it's no problem if you are not able to rebase your work. It was me who broke the process for the pull request here ;)

I have made some tests to add nodes within the course navigation with the before key setting, for example to place an "editing" node directly before the participants node (see this example on https://github.com/moodleuulm/moodle-local_boostnavigation/blob/LMS-3594-beforekey/lang/en/local_boostnavigation.php#L125). This has worked well.

I see your edge cases, however I am afraid that we must not forget the performance impact of searching just another node on every page load. So, the edge cases have to be considered wisely.

My proposal for the way forward would this:

Does this make sense to you?

Cheers, Alex

jrm-unamur commented 5 years ago

Thanks again for prompt feedback, and yes it makes sense). Still I'm interested in the git commands you wanted me to do ;-)

It is normal that using participants as beforekey works since it's a direct child of the current course node... In system context, only "home" and "mycourses" are direct child of the node used a base (myhome) (so calendar and privatefiles cannot be used without checking deeper) In course context default nodes (participants, competencies and grades) all work. Knowing that I'll try to handle edge cases with fine tuning to avoid as much as possible performance impact.

As your are about to publish a new release, may I advise you to copy custom node documentation in admin custom nodes areas . So far the only document title, link and supported languages.

Enjoy your weekend Jean-Roch

abias commented 5 years ago

Hi Jean-Roch,

Thanks again for prompt feedback, and yes it makes sense

as discussed, I have now merged my branch into master on https://github.com/moodleuulm/moodle-local_boostnavigation/commit/e304bdf3ea7f96df95e5a02968c6753dd1ed0823

Still I'm interested in the git commands you wanted me to do ;-)

Well, let's forget my proposal to rebase your latest changes onto my development branch. But I can explain what to do if you would want to rebase your latest changes onto my updated master branch.

I assume that you did something like git clone git@github.com:jrm-unamur/moodle-local_boostnavigation.git boostnavigation before to clone your Github fork to your machine.

Then you would have to do more or less this:

cd boostnavigation
git checkout master
git remote add upstream git@github.com:moodleuulm/moodle-local_boostnavigation.git
git fetch upstream
git rebase upstream/master
git checkout beforekeyparam
git rebase master

This will start the rebase process, but this will most probably also create conflicts as your latest changes don't apply cleanly anymore to my master branch.

You could then deal with the conflicts and finish the rebase. But in this case, I would propose if you just create a new branch, apply your outstanding changes manually and submit a new pull request. That's why I am closing this one here.

As your are about to publish a new release, may I advise you to copy custom node documentation in admin custom nodes areas . So far the only document title, link and supported languages.

Have you seen the notice on https://github.com/moodleuulm/moodle-local_boostnavigation/blob/master/lang/en/local_boostnavigation.php#L93 for admin nodes? The language pack is already a desaster regarding string duplication, I would rather not want to duplicate these descriptions anymore. I hope you understand.

Cheers, Alex