Add CSS classes to Wegue toolbar buttons

[chrismayer]

Adds CSS classes

• wgu-toggle-button
• wgu-geolocator-btn
• wgu-localeswitcher-btn
• wgu-zoomtomaxextent-btn

to module related buttons in order to easily style the automatically created Wegue module activator buttons (analogous to wgu-map-button).

[fschmenger]

Hi Christian, sorry for being so quiet lately as other work as keeping me bottled up.
I just like to point out that LocaleSwitcher is technically a menu button. So maybe you should assign your class to either both throughout the code (and possibly find another class name) or just to toggle buttons.
I hope you are well again!
Cheers Felix

[chrismayer]

Hi @fschmenger, good to read from you!

Yes I also stumbled upon the the fact that LocaleSwitcher is not a classical ToggleButton, although it behaves like for the user. But I agree to see it from the technical side and I splittet this up a bit. wgu-toggle-button is only added to ToggleButtons and other module related button implementations (found 2 more) are getting separate CSS classes, so they are addressable in a distinguishable way.

Would you please give it another look and approve, if you're OK with the change?

[fschmenger]

Hi Christian,
from just looking at the code this should be fine.

3 things to consider from my part:

• Should we add 4 empty rules to wegue.css, just as a placeholder?
• Another option could be to use just like 3 top-level classes like e.g. wgu-toggle-button, wgu-menu-button and wgu-action-button to kill several birds with one stone. Keep in mind that we already have theming for similar purposes - although it doesn`t really go beyond colorization for now.
• Maybe have a second look that you caught everything, e.g. what about the main menu overflow button?

Anyway approved...

[chrismayer]

Thanks for your feedback @fschmenger. I decided to go for the way of having more generic CSS classes:

• wgu-toggle-button
• wgu-menu-button
• wgu-action-button

to be more flexible and future proof here.

Theming was not sufficient for my requirement, so adding this classes was necessary to modify the auto-created Wegue toolbar buttons.

[sronveaux]

Hi team,

Just a word to say that, indeed, the selected solution as of now is the one I personally prefer too. This can be really useful and I like the fact that button classes are defined in terms of usage. Even though class names should not reflect pure technical points, they reflect behaviours here which is totally fine IMHO.

Even though I'm not a big fan of this and think it is a bit overkill, I just wanted to add that some best practices would recommend to define both classes on each button (for example, class="wgu-action-button wgu-geolocator-button"). This is already the case for the overview map button (class="wgu-map-button wgu-overviewmap") and is often done in frameworks (for example, the ZoomToMaxExtent button already has classes v-btn v-btn--icon and v-btn--round from Vuetify on top of which you have the opportunity to add yours).
As I said, I'm not a big fan of this and don't say it should be done that way, just wanted to let you think about it and choose what you think would be the best.

Concerning Felix's idea of putting a placeholder in the CSS files, this would imply to put them in the app-starter as in a perfect world, we should not change what's inside the src folder. But this file is currently empty. So should we begin to fill it? Are there other classes that should be there too? Should it be documented somewhere? This interesting idea could lead to a new issue to open and discuss...

I nevertheless approve it as is...

[chrismayer]

Thanks for your review and the feedback @sronveaux.

The additional classes like class="wgu-action-button wgu-geolocator-button" could be some overkill, as you already stated. If we have a situation where adding these additional classes becomes necessary we should add them at this time.

Some quick words about adding empty rules: I don't think that putting empty CSS rules is a good idea. This could really mess up in the future. Since this has some documentation character we maybe find a better place in the future. In case further discussion is wished, please open up an issue.

[chrismayer]

I am going to merge now, thanks for your valuable input @fschmenger and @sronveaux 👍