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

Simplify glitch-soc's theming system #2697

Conversation

ClearlyClaire
Copy link

@ClearlyClaire ClearlyClaire commented Apr 27, 2024

glitch-soc's over-engineered theming system has been a recurring pain when merging upstream changes, and this is getting significantly worse with upstream making changes to its theming system.

Therefore, this PR aims to simplify it a great deal and bring it closer to upstream.

As upstream's theming system is undergoing changes, glitch-soc's theming system may change as well in the near future, even after this PR gets merged.

Breaking changes

(The following is still subject to change)

For end users

Hopefully, none.

For skin (purely SCSS changes) authors

Although nobody used that to my knowledge, skins allowed multiple entry-points. This has now changed, a skin has a single entry point, with one of the following being used:

  • app/javascript/skins/:flavour/:skin.scss
  • app/javascript/skins/:flavour/:skin.css
  • app/javascript/skins/:flavour/:skin/common.css
  • app/javascript/skins/:flavour/:skin/common.scss
  • app/javascript/skins/:flavour/:skin/index.css
  • app/javascript/skins/:flavour/:skin/index.scss
  • app/javascript/skins/:flavour/:skin/application.css
  • app/javascript/skins/:flavour/:skin/application.scss

For most, if not all skins, this should require no change whatsoever.

For flavours (javascript pack changes) authors

The following changes have been made to theme.yml:

  • pack_directory is no longer optional
  • fallback is no longer supported
  • pack is no longer supported, entrypoints are directly identified by their filename
  • the root attribute signed_in_preload has been added to fulfill the purpose of the preload attribute that was previously pack-specific

In addition, the following changes have been made:

  • some packs have been reorganized to match upstream's usage:
    • home is now called application
    • auth does not exist anymore, its contents are in public and two_factor_authentication
    • settings does not exist anymore, its contents are in public
  • the common pack is unconditionally loaded in addition to other packs

Additional internal changes

  • to match upstream, pack selection is now done in views and no longer in controllers
  • the implicit core flavor has been removed
  • use_pack, @core and @theme have been removed, replaced with new helpers: preload_locale_pack, flavoured_javascript_pack_tag and preload_signed_in_js_packs

@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/refactor/revamp-theming-system branch 20 times, most recently from 2958a6e to c9cf24f Compare April 28, 2024 12:56
Have all flavors implement everything they need instead.
Remove the `fallback` property and do not fallback to using another flavour
when a pack is not available in the selected flavour.

Flavours should define all packs, and should they wish to piggy-back on
another one, they can import that other one's pack explicitly instead.
@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/refactor/revamp-theming-system branch 2 times, most recently from 6a0f437 to 4994415 Compare April 28, 2024 13:26
Load the `common` style pack, and then charge the style pack for the current
skin, independent from any selected JS pack.
Packs are now loaded from views, just like upstream, and are
identified by their filenames. The definition of `theme.yml` has
changed as such:
- `pack_directory` is now required
- `pack` is now unused
- `signed_in_preload` has been introduced
@ClearlyClaire ClearlyClaire force-pushed the glitch-soc/refactor/revamp-theming-system branch from 4994415 to 7208edb Compare April 28, 2024 13:47
@ClearlyClaire ClearlyClaire marked this pull request as ready for review April 28, 2024 14:10
@ClearlyClaire
Copy link
Author

@listerine420 I intend to merge this PR shortly, as it will make merging upstream changes a lot easier, but I see you've reacted with a thumbs down emoji. Is there a specific issue with this PR that I should be aware of and possibly address before merging?

@ClearlyClaire ClearlyClaire merged commit 6f74ede into glitch-soc:main Apr 30, 2024
34 checks passed
@jippi
Copy link

jippi commented May 13, 2024

Thanks for this simplification! It's so much better - I never quite figured out how the old system worked, so copied it most of the stuff from the glitch theme (without knowing what it did) - the new setup is way clearer!

+++ b/docker/mastodon/flavours/bird-ui/theme.yml
@@ -1,28 +1,22 @@
-pack:
-  admin:
-    - admin.tsx
-    - public.tsx
-  auth: public.tsx
-  common:
-    filename: common.js
-    stylesheet: true
-  embed: public.tsx
-  error: error.js
-  home:
-    filename: application.js
-    preload:
-      - features/compose
-      - features/home_timeline
-      - features/notifications
-  mailer:
-  modal:
-  public: public.tsx
-  settings: public.tsx
-  sign_up: sign_up.js
-  share: share.jsx
+# (REQUIRED) The directory which contains the entry point files.
+pack_directory: app/javascript/entrypoints
 
-pack_directory: app/javascript/packs
 
-fallback:
+# (OPTIONAL) A file to use as the preview screenshot for the flavour,
+# or an array thereof. These are the full path from `app/javascript/`.
+screenshot: images/bird-ui-preview.jpg
-screenshot: images/bird-ui-preview.jpg

@ClearlyClaire
Copy link
Author

@jippi I am a bit confused by your use of the theming system. How does bird-ui differ from the vanilla flavor?

@jippi
Copy link

jippi commented May 13, 2024

This bird-ui doesn't really differ beyond just styling (no functionality)

I (probably misused?) the flavor system to give it a more prominent place in the profile UI than a regular "style" since folks had a hard time finding it in the "vanilla" section, and I wanted to give it a screenshot :)

If there is a better way to achieve this, I'm all ears :D

Image 2024-05-13 at 1 37 38 PM jpg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants