Add CSS classes to Wegue toolbar buttons

wegue-oss / wegue

Template and components for webmapping applications with OpenLayers and Vue.js

BSD 2-Clause "Simplified" License

93 stars 41 forks source link

Add CSS classes to Wegue toolbar buttons #382

Closed chrismayer closed 2 months ago

chrismayer commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

chrismayer commented 2 months ago

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 commented 2 months ago

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