Calling the SiteCustomization API from a plugin


#1

ping @radq and @lightyear

Hi guys, we’re having some issues getting our reskin/customisation plugin working. @sam asked us to post here so that we can get some help over the weekend because he’s heading away. Thanks.

My front end guy has the plugin partially working.

If you have a look at https://github.com/sitepoint/discourse-theme/you can see that we have a plugin.rb file in which we load additional SCSS files. At the end of the file is a comment that includes the content of the Site Customization form field as we use it (with “Do not include standard style sheet” checked). What we’d like to achieve is to be able to call the SiteCustomization API from the plugin and inject the variables from there, rather than having to use the form in the Admin section.
Could you please provide guidance on how to do that?

Also, the generated stylesheet file in public/uploads/stylesheet-cache currently combines the styles in a strange order. Most of the styles are applied correctly (i.e. default styles come first, and our override styles are lower in the file), but in the case of e.g. header.scss, the default style come after our override (search for the “.d-header {” rule), which is not desirable, because for example our definition of the header background color is overwritten back to default. How can we ensure that our plugin styles are always loaded after the default styles? It looks like the order is somewhat random now.
This issue was already present before we started using the SiteCustomization feature: in the browser’s dev tools’ Network tab, the our header.css would come up earlier than discourse’s default header.css.

Thanks very much.


(Benjamin Kampmann) #2

Why? I don’t understand this part. I read in the plugin.rb that you want to have the custom variables.scss to be loaded instead of the original. But in there you only specify new-variables (suffixed with -SP), so none of the existing variables is overwritten and no other scss will be using them aside from yours, correct? Well, then just use the normal “scss”-import way of doing things with relative paths. In your *.scss instead of:

@import "common/foundation/variables";

do

@import "./common/foundation/variables";
/* or */
@import "../common/foundation/variables";

The import-path is always looking for the assets core-application-first. By making the path relative, it will only look in the specific folder. I think that should do it for you. (Another way is also to achieve the same is by using plugin-specific file-names so that not the global ones get used).

I made a patch here that moves some files around but most importantly uses one application.scss that then relative imports to get your whole design in order and fix these problems.

It looks somthing like this on my dev-machine then:


Yes, currently the plugins are loaded in common.scss which is loaded before both desktop/* and mobile/*. Aside from this being code-reuse I don’t see a good reason why that is the case though. If that is okay with @radq and @sam, I can create a pull-request moving this call after the others respectively.


#3

I’ll get Boris to address this with you today. Thanks for your response.


(borhub) #4

Hi Benjamin,
thanks for the patch.

Unfortunately, your understanding is incorrect, the reason why we need the SiteCustomization API is because we don’t only specify new variables, but need to also redefine/overwrite the values of the existing default variables (e.g. $nav-pills-background-color-active) and have the new values be used in the Discourse’s default theme scss files. This can be achieved by first defining our custom values in the SiteCustomization form, and only later including the line @import "desktop" (a new feature since April), and checking the “Do not include standard style sheet” checkbox), so that the Discourse default scss files are processed only after we set our own values for the existing variables.

For example:

// set a variable that's used in Discourse's default theme to our own value
$base-font-family: "Times", "Helvetica Neue", Helvetica, Arial, sans-serif;

// process Discourse's default theme only after variables have received our custom values
@import "desktop";

// continue with our theme styles and overrides of the default theme (in reality we want to do this by registering our .scss theme files in the plugin, but that currently has the issue with the order)
body {
    background-color: #bada55;
}

#topic-list {
  > tbody > tr {
    &:nth-child(odd) {
      background-color: transparent;
    }
  }
}

The reason we want to call the SiteCustomization API from the plugin is version control and having all things in one place.
So I wonder how we can “populate the Site Customization form” from within the plugin?

Yes having the plugin styles imported only after the default desktop and mobile is precisely what’s needed.
It looks like currently with plugins and plugins_mobile, the plugins thing serves a double role of “common” as well as “desktop”. How about having this separated to: 1) a new plugins_common thing (to be imported in common.scss), 2) then a new plugins_desktop thing imported in desktop.scss, and 3) the already existing plugins_mobile in mobile.scss?

Thanks!
cc @santouras


(Benjamin Kampmann) #5

Okay, I think I understand what you are going at. You are using the SiteCustomization API to prevent the defaults from being loaded before you’ve added your variables. Now I understand where you are coming from.

And after thinking about it for the day, I come to the conclusion that I don’t think this is should be approach to be taken imho. The SiteCustomization-Part is for the administrator and IMHO should always overwrite what other plugins might have shipped. Even if a plugin comes full-featured, the Administrator at the Frontend should always have last saying. Ergo, messing with that from within a plugin sounds like a messy – not to say wrong – approach to me.

I would recommend the following approach instead:

  • add an option to register_assets to allow specification of variables
  • load variable-scss-plugins-files at the end of the current variables.scss, allowing them to set and overwrite everything stated so far
  • move the call for the plugin-scss out from the common.scss and after desktop/* and mobile/* respectivly
  • while on-it, add a ‘desktop’-option-flag to the scss preventing it from being loaded in the mobile-case (to be backwards compatible but allow smaller scss-footprints on mobile)

This, together with the changes I gave you earlier should totally be sufficient to allow your plugin to set/overwrite the default values of variables while not having to get into the SiteCustomization, which is totally under the domain and control of the website administrator allowing them to overwrite even what plugins decided to overwrite.

If that sounds good and I get an okay from @sam about this, I’m happy to make a Pull-Request as described.


(borhub) #6

Sounds good!

load variable-scss-plugins-files at the end of the current variables.scss, allowing them to set and overwrite everything stated so far

An even fancier way to do this would be to load variable-scss-plugins-files before foundation/variables.scss and rely on the !default suffixes in foundation/variables.scss (they are almost on every variable now, would need to be added only to the few vars that don’t currently have it). What this would achieve is even less SCSS needed to be written on the plugin part (e.g. define just $red: lightgreen; and then there’s no need to redefine $nav-pills-color-active: again in the plugin), but a convention of having !default everywhere in foundation/variables.scss would need to be adhered to.

I agree with your point about not giving plugins the ability to override the Site Customization done by the site’s admin. The SiteCustomization API was the closest currently existing feature relevant to the use case we’re exploring (trying to build a Discourse theme as a plugin), so that’s why we started there after Sam suggested to have a look at it (in a private topic on our testing forum).

move the call for the plugin-scss out from the common.scss and after desktop/* and mobile/* respectivly

while on-it, add a ‘desktop’-option-flag to the scss preventing it from being loaded in the mobile-case (to be backwards compatible but allow smaller scss-footprints on mobile)

How about a full split to “common”, “desktop” and “mobile” as I suggested in my previous post? (Old plugins calling the register_asset(SCSSPATH) with no arguments would be injected as common, new plugins could call something like register_asset(SCSSPATH, 'common'), register_asset(SCSSPATH, 'desktop'), register_asset(SCSSPATH, 'mobile'). Fully responsive (theme) plugins would inject styles just into common.

Relevant to the paragraph above, it might also be useful if a plugin was able to completely turn off the default stylesheets (an ambitious theme plugin might want to start styling the site from scratch). I guess this would need to be indicated to the site admin in the Site Customization UI in some way, e.g. by programmatically checking the “Do not include standard style sheet” and graying it out, with a note that an existing plugin has already disabled the standard stylesheet, or similar.


(Benjamin Kampmann) #7

That isn’t the case in the approach taken. All variables would be loaded and then plugin-variables would be loaded. This allows to overwrite existing variables, but everything already there, well… will just be there. It will not replace the files, just overwrite some variables. I think that is a prettier approach then requiring the default keyword, which is easily overlooked or forgotten in reviews later.

I thought about that. But that wouldn’t have solved the variables problem and therefore I am not sure about why one wants that. Now, you’ve given the fair point of fully-responsive-plugins-scss (though I stick to the point that file-size for mobile does matter, especially for parsing so it is always preferably to remove anything from the mobile styles that aren’t needed), but that wouldn’t explain why “common” needs to be loaded before the other paths (and I find the name misleading for the purpose). And it would create backwards incompability.

What if, to make that approach happen, we have two calls after desktop/* and mobile/* respectively:

@import plugins;
@import plugins_mobile;

and

@import plugins;
@import plugins_desktop;
  • Backwards compatible ✓
  • allows overwriting of default styles through plugins: ✓
  • allows plugin author to decide for mobile/desktop-split: ✓
  • allows plugin author for common scss/css approach: ✓

(Benjamin Kampmann) #8

So, cleaning up,

I could make a Pull-Request doing the following (potentially today):

  • add a variables-option for register_assets
  • load those at the end of variables
  • add a desktop-option for register_assets, add plugins_desktop to sass-lookup.
  • move @import plugins; into desktop.sass and mobile.sass
  • add @import plugins_desktop; into desktop.sass using the newly added desktop-marked assets

Giving more flexibility to theme-plugins.

What do you think of that approach, @sam, @radq , @zogstrip, @codinghorror ? Worth pursuing?


(Jeff Atwood) #9

I think @neil would have to respond, since he is working on the E-Z themer at the moment.


(Neil Lalonde) #10

Setting the value of variables like $nav-pills-background-color-active is exactly what the themer will allow you to do. (see here)

For adding new variables, we’d still need the solution described here.

So, go ahead with your work @lightyear. I haven’t changed the way scss is loaded yet. With the themer, plugins would be able to add their “theme” (the values for the variables) and they’ll show in admin, or just overwrite the variables scss (the discussion above).


(Benjamin Kampmann) #11

Thanks, @neil. I was wondering. Is there a way to maybe load the SASS-File and learn about all variables existing and show them in the Admin-Interface (similar as we do with the site_settings)? Then an admin could also easily adapt the colors of a theme or other plugins if specified properly by the plugin.


Anyhow, here is the pull-request:

https://github.com/discourse/discourse/pull/2292

@borhub, could you confirm this fixes this/your case?


(Kane York) #12

How about, instead of registering a “scss variables” file, the plugin registers the actual variables instead?

sass_color :nav-pills-background-color-active, "#B4ED86"
sass_variable :poll-bar-height, "25px"

Done this way, there’s an obvious way to collect the names and defaults. And maybe even group them by plugin in the color chooser.


(Neil Lalonde) #13

That’s happening now for the configurable colors. See here and here. :smile: You can get a list of the ones that can be edited with ColorSchemeColor::BASE_COLORS.keys, if that’s what you need.

sass_variable looks handy for arbitrary variables like in your example.

Here’s what I’m thinking of for theming:

  1. Add a color scheme:
register_color_scheme 'Purples', {
  primary_background_color: 'EEE0E5',
  secondary_background_color: {hex: '2E0854', opacity: 50},
  tertiary_background_color: '4B0082'
}
  1. Add a theme (colors, css, html):
register_theme 'Such Purple',
  color_scheme: {
    primary_background_color: 'EEE0E5',
    secondary_background_color: '2E0854',
    tertiary_background_color: '4B0082'
  },
  stylesheet: a,        # filename or inline scss
  header: b,            # filename or inline html
  mobile_stylesheet: c, # filename or inline scss
  mobile_header: d,     # filename or inline html

(borhub) #14

Hi @lightyear, I just pushed a new version of our plugin that uses your new API. I can confirm it works great. Thanks for the awesomely quick implementation!


(borhub) #15

Just adding a note so that someone hopefully doesn’t run into the same situation:
I started building our theme plugin with the desktop version first. But because I initially didn’t specify :desktop in the register_asset calls for the files in stylesheets/desktop/, we ended up with a broken Mobile View for months (posts were too wide, etc).
So be careful to not get misguided by thinking that if you use paths like stylesheets/desktop in your plugin, it will have an effect on which stylesheet the file gets compiled into. Always add the :[view] flag if the stylesheet is specifically for desktop or mobile :wink:

# header: device-independent item colors and reset of badges back to inline display in hamburger menu
register_asset "stylesheets/common/base/header.scss"
# header: desktop - expand header to full viewport width, styling of custom links and logo
register_asset "stylesheets/desktop/header.scss", :desktop
# header: mobile - reduced paddings and icon widths, SP site links hidden
register_asset "stylesheets/mobile/header.scss", :mobile