moodle-an-hochschulen / moodle-theme_boost_union

Theme Boost Union is an enhanced child theme of Boost which is intended, on the one hand, to make Boost simply more configurable and, on the other hand, to provide helpful additional features for the daily Moodle operation of admins, teachers and students.
GNU General Public License v3.0
55 stars 49 forks source link

feat: show course module icons in course index, resolves #16 #554

Closed christianwolters closed 4 months ago

christianwolters commented 5 months ago

Hi,

I'll adress grunt and codechecker asap.

Cheers Christian

lucaboesch commented 4 months ago

Thanks for your proposal, @christianwolters !

Allow me some thoughts about your work. You offer two options which are carefully programmed for. From default to the left hand side to "End of line" (middle) and "Icon" (right)

image

Now, for me there are options left out, and I don't know whether because it is impossible to accomplish or why else but I'd like to mix them into the game. That would be (default to the left hand side again) "Start of line" which also could be called "in front of the icon" (middle) and "after the icon" (right)

image

So, in the end, the four options could be: "in front of the icon", "after the icon", "end of line", and "colour in the icon".

What do you think?

Best, Luca

christianwolters commented 4 months ago

Hi @lucaboesch,

thanks for taking the time to look at this.

Regarding the "left out" options:

That being said, I like the idea of offering more display options (and also i really like your option naming: "in front of the icon", "after the icon", "end of line", and "colour in the icon").

Right now, I'm thinking that these options could be implemented, but there is certainly a need for some thinking and refactoring, which would block this PR.

So I'd like to move your ideas into a new issue, so they become the next iteration of this feature?

Cheers Christian

lucaboesch commented 4 months ago

Yes, sure, it is very welcome to add the two other options as follow-up issues. It is unlikely they would have been in in time for our upgrade February 14th, 2024, anyway 😜.

abias commented 4 months ago

Hi @christianwolters , many thanks for contributing this feature!

And thanks @lucaboesch for your initial thoughts!

I have now taken this over and made an integration review.

Doing this:

Unfortunately, I was not able to push my review changes to your fork @christianwolters.

That's why I will close this PR now and will create a follow-up PR on #562.

Cheers, Alex