canonical / vanilla-framework

From community websites to web applications, this CSS framework will help you achieve a consistent look and feel.
https://vanillaframework.io
GNU Lesser General Public License v3.0
798 stars 163 forks source link

Percy examples combination - `patterns/side-navigation` #5170

Closed jmuzina closed 6 days ago

jmuzina commented 1 week ago

Combined patterns/side-navigation examples.

Deletes theme-specific side-navigation examples.

Interactivity is disabled for the combined view, as the script for this component was not written in such a way that it can be combined / imported multiple times yet.

QA

webteam-app commented 1 week ago

Demo

Jenkins

demos.haus

pastelcyborg commented 1 week ago

Similarly to the navigation PR, I'll defer to @bartaz since I may not have all the context to fully sign off on this.

bartaz commented 1 week ago

Something is broken in this one. It may have some hardcoded dark background originally?

image
jmuzina commented 1 week ago

@bartaz

Something is broken in this one. It may have some hardcoded dark background originally? image

This also happens on production when you view this example in light theme.

It comes from _layout-JAAS which is trying to use jinja variables to set the icon color. However color theme changer works in JS, not jinja

bartaz commented 1 week ago

Oh, OK. So, these conditionals {% if is_dark %}is-light{% endif %} on icons are not needed anymore, as they should be themed automatically without any need for class name right now:

https://github.com/canonical/vanilla-framework/blob/3efcf83eedff4fee69e7be25d111413353fa72e3/templates/docs/examples/patterns/side-navigation/_layout-JAAS.html#L13C32-L13C67

jmuzina commented 1 week ago

Oh, OK. So, these conditionals {% if is_dark %}is-light{% endif %} on icons are not needed anymore, as they should be themed automatically without any need for class name right now:

https://github.com/canonical/vanilla-framework/blob/3efcf83eedff4fee69e7be25d111413353fa72e3/templates/docs/examples/patterns/side-navigation/_layout-JAAS.html#L13C32-L13C67

@bartaz Removed those conditionals & also rebased to remove merge conflicts!

jmuzina commented 6 days ago

Blocked pending merge of #5165

jmuzina commented 6 days ago

5165 has been merged; script include if checks were replaced with script blocks