Add new items to disable

halbo5 commented 7 years ago

Hide badges and Competencies if there are no badges and no competencies. Add a new function to hide nodes in course navigation (filters, Outcomes, Publish, ...) french translation

abias commented 7 years ago

Hi Alain,

thanks for your pull request, this is greatly appreciated.

Some quick feedbacks at first glance:

The PR contains a file db/settings.php which is a copy of settings.php. I assume this sneaked in by mistake, please remove it.
The PR contains the french translation in lang/fr/local_boostnavigation.php. Please see https://github.com/moodleuulm/moodle-local_boostnavigation/blob/master/README.md#translating-this-plugin, act accordingly and remove this file from the PR.
If you don't mind, please the new settings to README.md.
Please have a look at Travis CI's test results on https://travis-ci.org/moodleuulm/moodle-local_boostnavigation/jobs/258533523#L530 ff. and fix your coding style.

After you have updated your PR, I will be happy to have a in-depth look at it and approve it.

Thanks, Alex

halbo5 commented 7 years ago

Thank you for your comments. I did'nt know the Travis CI's tests, so I learned something. I deleted settings.php, this was a bad copy/paste. I deleted the French translation and added translation on AMOS. I completed the README.md file. I corrected the Travis test.

Best,

Alain

abias commented 7 years ago

Hi Alain,

thanks for improving your pull request and sorry for not getting earlier back to you.

I was now able to have a deeper look at your code and would like to give some more feedback.

There were multiple new functionalities in your pull request and I have to handle them separately.

Removing the Badges course navigation item if there are no badges in the course is a nice idea and would be welcomed here, too. As you can configure badges in the course cog menu, you can still get to the badge configuration as a teacher. I will finally add this to the plugin soon.
Removing the Competency navigation item if there are no competencies in the course and adding it to the course cog menu is a nice idea, too. However, I struggle with the way you are searching for this navigation node in https://github.com/moodleuulm/moodle-local_boostnavigation/pull/4/files#diff-828043dc9f8d10ccd58ff8b96297794fR81, this is really fragile as it's not guaranteed that the competencies node gets this key. I have created https://tracker.moodle.org/browse/MDL-59879, please let me wait until this is integrated into core until I proceed here.
Regarding the removal of multiple elements from the cog icon, I am sorry that I would like to deny these features. I fully agree that the cog menu is a quite bad solution and it should be improved. However, we not only struggle with the fact that the cog menu does contain unnecessary items, but mainly with the fact that it does not contain important items. Even Moodle HQ has accepted this fact and is currently working on improvements to this course menu. For our local installation, we have created a Boost child theme which replaces the cog menu with a in-course settings menu (see https://github.com/moodleuulm/moodle-theme_boost_campus#in-course-settings-menu) as a intermediate solution until Moodle HQ has solved this menu globally. Unfortunately, I was not able to integrate this feature into local_boostnavigation easily, that's why it ended in our theme. And that's why I can't accept these cog menu changes as we won't be able to maintain them in the long run. Sorry...

Summing up: Please stay tuned some more days until I integrate feature 1 and 2.

Thanks, Alex

halbo5 commented 7 years ago

Thank you for your answer. I understand why you don't want to remove elements from the cog icon.

Best,

Alain

2017-08-21 1:20 GMT+02:00 Alexander Bias notifications@github.com:

Hi Alain,

thanks for improving your pull request and sorry for not getting earlier back to you.

I was now able to have a deeper look at your code and would like to give some more feedback.

There were multiple new functionalities in your pull request and I have to handle them separately.

-

Removing the Badges course navigation item if there are no badged in the course is a nice idea and would be welcomed here, too. As you can configure badges in the course cog menu, you can still get to the badge configuration as a teacher. I will finally add this to the plugin soon.

Removing the Competency navigation item if there are no competencies in the course and adding it to the course cog menu is a nice idea, too. However, I struggle with the way you are searching for this navigation node in https://github.com/moodleuulm/moodle-local_boostnavigation/ pull/4/files#diff-828043dc9f8d10ccd58ff8b96297794fR81 https://github.com/moodleuulm/moodle-local_boostnavigation/pull/4/files#diff-828043dc9f8d10ccd58ff8b96297794fR81, this is really fragile as it's not guaranteed that the competencies node gets this key. I have created https://tracker.moodle.org/ browse/MDL-59879, please let me wait until this is integrated into core until I proceed here.

Regarding the removal of multiple elements from the cog icon, I am sorry that I would like to deny these features. I fully agree that the cog menu is a quite bad solution and it should be improved. However, we not only struggle with the fact that the cog menu does contain unnecessary items, but mainly with the fact that it does not contain important items. Even Moodle HQ has accepted this fact and is currently working on improvements to this course menu. For our local installation, we have created a Boost child theme which replaces the cog menu with a in-course settings menu (see https://github.com/moodleuulm/moodle-theme_boost_campus#in- course-settings-menu https://github.com/moodleuulm/moodle-theme_boost_campus#in-course-settings-menu) as a intermediate solution until Moodle HQ has solved this menu globally. Unfortunately, I was not able to integrate this feature into local_boostnavigation easily, that's why it ended in our theme. And that's why I can't accept these cog menu changes as we won't be able to maintain them in the long run. Sorry...

Summing up: Please stay tuned some more days until I integrate feature 1 and 2.

Thanks, Alex

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/moodleuulm/moodle-local_boostnavigation/pull/4#issuecomment-323618504, or mute the thread https://github.com/notifications/unsubscribe-auth/AApMCtmrcCvFmKdWleKk7vvyzuRBTXj5ks5saL8jgaJpZM4OmiGr .

abias commented 6 years ago

Hi Alain,

it's been a while since you created this PR and I apologize for my lack of feedback. I was finally able to include the functionality which you have built. For some structural reasons and because of the changes I outlined above, I commited it myself on https://github.com/moodleuulm/moodle-local_boostnavigation/commit/71b21570b235337cca3a85a9daa412e06d51af33 and will now close this PR without merging it.

Thanks, Alex

moodle-an-hochschulen / moodle-local_boostnavigation

Add new items to disable #4

Removing the Badges course navigation item if there are no badged in the course is a nice idea and would be welcomed here, too. As you can configure badges in the course cog menu, you can still get to the badge configuration as a teacher. I will finally add this to the plugin soon.