Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Add order to outputted stylesheet link tags #13735

Merged
merged 10 commits into from
Jul 15, 2021
Merged

Conversation

pmusaraj
Copy link
Contributor

@pmusaraj pmusaraj commented Jul 14, 2021

Theme and component stylesheets have so far been loaded without a specified order. This causes issues. Especially on sites with multiple themes and multiple components, styles from components would sometimes take precedence over the base theme and sometimes it would be the other way around.

This PR orders the theme stylesheets thusly:

  • remote theme components (ordered alphabetically)
  • remote main theme (if applicable)
  • local theme components (ordered alphabetically)
  • local main theme (if applicable)

Note that stylesheet ordering is not yet enabled by default, for now this behaviour is behind a hidden site setting: order_stylesheets.

To use this, do the following in the Rails console:

pry(main)> SiteSetting.order_stylesheets = true

pry(main)> Stylesheet::Manager.clear_theme_cache!

This PR also removes an extra layer of caching. We cache the full array of stylesheets for a specific target, and we also cached each stylesheet path, which is redundant.

@pmusaraj pmusaraj requested a review from tgxworld July 14, 2021 19:17
@eviltrout
Copy link
Contributor

This looks good to me but could it possibly break existing themes where they depended on the previous order? I agree this order is better, for the record. I just want to figure out if any forums will be affected negatively.

@pmusaraj
Copy link
Contributor Author

Yes, it can potentially break existing sites. We currently don't have a load order, though, so this would have caused issues sooner or later anyway...

I will test this on a few sites with many themes and components before merging. Also, the main risk here are sites that have styles in a component that are supposed to override styles in the parent theme.

@eviltrout
Copy link
Contributor

Thanks I think it's better to have an order than not, but we should be aware of the risk and have a plan of action when merging.

- remote components (a to z)
- remote parent theme (if applicable)
- local components (a to z)
- local parent theme (if applicable)
lib/stylesheet/manager.rb Outdated Show resolved Hide resolved
Comment on lines 159 to 161
childA = hrefs.select { |href| href[:theme_id] == child_theme.id }.first
childZ = hrefs.select { |href| href[:theme_id] == z_child_theme.id }.first
childR = hrefs.select { |href| href[:theme_id] == child_remote.id }.first
Copy link
Contributor

@tgxworld tgxworld Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're mixing camelCase and snake_case for the variables in this spec. Usually we default to snake_case for variable names in Ruby.

Copy link
Contributor

@tgxworld tgxworld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor code style comments but the PR looks good to me otherwise. Regarding how we would deploy this, I recommend just putting the sorting behind a hidden site setting first and then roll it out slowly across our hosting.

pmusaraj and others added 6 commits July 15, 2021 10:00
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
@pmusaraj pmusaraj merged commit a23153f into master Jul 15, 2021
@pmusaraj pmusaraj deleted the order-stylesheets branch July 15, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants