Problem/Motivation

Drupal core modules typically provide minimal markup and CSS (just enough to work) and themes take care of adding classes and styling beyond those essentials. Here's a very generalized overview of how different assets are themed

  • Twig templates a theme provides a template with the same filename, which overrides the default template. The override can be minimal (such as Classy adding a few classes), or a more extensive rebuild.
  • Render Arrays _preprocess() hooks are used to alter render array contents. claro.theme makes extensive use of preprocess functions to add theme-specific classes to render elements.
  • CSS CSS can be replaced (via override) or extended (via library extension), both of which are configured in a theme's info.yml file.
  • JavaScript generated Markup Some markup is dynamically generated by JavaScript, so does not come from a render array or twig template.

Svelte doesn't cleanly belong to any of the above, but ideally the app could be similarly themed. A .svelte file is a combination of JavaScript, HTML templates and CSS that is compiled to JavaScript.

Proposed resolution

  1. First, refactor many of our values into CSS custom properties. This will allow easy overrides for other themes.
  2. Second, remove the hashed classnames introduced by the Svelte build, so that markup looks more Drupal-y. Using BEM syntax where possible, this aligns better with Drupal markup today. We may need to refactor some of our existing CSS to this end.
  3. Third, allow for the insertion of markup (using Drupal.theme, for example) into component prefixes and suffixes where there's a good opportunity (e.g. the card component).
  4. Fourth, and only if there is great clamoring for it, allow users to override individual Svelte files (or pieces of them), though this introduces a build step. Note: a really motivated person could already today fork and rebuild the front-end to their heart's content.

Issue fork project_browser-3285889

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

The add-conditions-in-preprocessor-approach is an attempt at the first approach mentioned in the issue summary: using Svelte compile preprocessors to add conditionals to templates that have identified overrides. It's doesn't currently There's some comments in there regarding where it is at and what additional steps would be needed.

And to restate: there's no agreed upon approach yet, this branch exists to build a prototype of one possible approach. Determining the best way to make Project Browser themeable is still very much a discussion.

bnjmnm opened merge request !176

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

The overrideAttributes MR proposes an approach that provides less themeability than render arrays or twig templates, while still giving themes the ability to have a resonable level of control over the appearance.

The Drupal.svAttr() function sends an array of attributes for potential processing. For example <div {...Drupal.svAttr('pbMainContainer', { class: 'container-1' })}> checks for the existience of a <code>Drupal.svAttr.pbMainContainer() function. If present, Project Browser will use the return value of that function to set attributes instead of the default values (which are { class: 'container-1' })

The MR currently demonstrates two overrides, but we could have this throughout the application

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Project Browser 2.0
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review

Ready for review (does contain some debug code, but the overall approach is ready)

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

Adding some clarity that there is code for TWO different approaches, as described above, but as there's just one open MR, you cannot easily see both.

For approach 1 (allow overriding whole Svelte components, but requires an app rebuild): https://git.drupalcode.org/issue/project_browser-3285889/-/compare/1.0.x...

For approach 2 (allow pushing classes down into the markup, does not require app rebuild): https://git.drupalcode.org/project/project_browser/-/merge_requests/176/...

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

I had a third hare-brained idea but I think I think might kind of work. Please help me compare and contrast with the other two available options. Here's my summary:

1. The first approach gives the most amount of control to the theme, but requires a rebuild of the main project browser app, and results in different / conditional markup.

2. The second approach sacrifices the maximum flexibility for the ability to provide its own classes, but does not require rebuilds (which I do like).

3. However, with second approach, is it not feasible for us to just provide one top-level class at the app level with the theme machine name... like on the App component or div wrapper, just provide `theme--olivero` or whatever? Then they can just style `.project-browser.theme--olivero anything { ... }`

4. A third approach. At the end of the day, the Svelte app is compiled to a bundle.js and a bundle.css. We provide it as a library, which means it could be overridden. Could we provide a tool which scans the current theme for .svelte files, fills in the rest of the holes with the "main" project browser, and recompiles the css/js into that theme's folder to serve their alternative app, fully-compiled? (I'm thinking of something as a yarn script that would copy the OG project browser code into temp path, then copy the overrides over those, then compile it, then copy the bundles to a specified location.)

...the above reminds me of the way Gin is a sort of permanent-enhancement of Claro.

Potential caveats/upsides/downsides of this approach:
- once you override, you've taken ownership. As things change, you might need to manually merge those changes into your override. (However, this doesn't seem radically different from how Twig templates are overridden now)
- it does give max flexibility - "alternative frontends" are entirely possible with a base provided.
- this flexibility increases the risk surface for breaking changes
- building this way just to provide your own styling/classes and not really changing the markup causes a large subset of overrides to maintain (for this reason, I might still suggest having my approach (3) above be also available

It would be nice to get this done as a potential third proof of concept.

The only other thing I looked at was potentially changing the paths of import statements during compile time, and though that is theoretically possible, it cannot use variables in the name of course: https://github.com/sveltejs/svelte/issues/6702 - there are some extensions that give you _some_ flexibility there, but the names need to be predictable: https://www.npmjs.com/package/@rollup/plugin-dynamic-import-vars

I think a total-app-rebuild works around these issues by merging the files in a temporary location to build the bundles.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Glad to see some take on this!

However, with second approach, is it not feasible for us to just provide one top-level class at the app level with the theme machine name... like on the App component or div wrapper, just provide `theme--olivero` or whatever? Then they can just style `.project-browser.theme--olivero anything

If this top level class was helpful it could be added automatically in BrowserController regardless of theme. It would make targeting project-browser specific styling pretty easy, but this would likely have to be in addition to another approach as there needs to be a way to implement a theme's existing styles. Adding Claro specific classes while in Claro lets use existing styles only when Claro is is in use instead of making every admin theme look like Claro and we don't need to recreate styles that are already present in Claro's CSS.

A third approach. At the end of the day, the Svelte app is compiled to a bundle.js and a bundle.css. We provide it as a library, which means it could be overridden. Could we provide a tool which scans the current theme for .svelte files, fills in the rest of the holes with the "main" project browser, and recompiles the css/js into that theme's folder to serve their alternative app, fully-compiled? (I'm thinking of something as a yarn script that would copy the OG project browser code into temp path, then copy the overrides over those, then compile it, then copy the bundles to a specified location.)

I tried this and it may be in one of the branches/histories. I was able to make entire .svelte file overridable, but I don't like the thought of someone having to reproduce an entire set of JS, HTML and CSS to make a small change to one of those. I spent some time looking into AST parsing to see if I could make specific sections overridable without success. I didn't get to a point where I could determine if this not happening was due to a technical or skills limitation.

Another approach that might be feasible but could take some adjusting is splitting the svelte files into theName.js.svelte, theName.css.svelte and theName.html.svelte files. The build process would then concatenate the three into a .svelte file, and using overriden versions if found in a theme.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

I don't necessarily dislike the idea of splitting the pieces out, because it keeps hands out of the js to some extent, which is probably desirable. But then I'm not sure if that's adding more work / burden to do theming if you are wanting to change styles and markup. Not sure what the most common use case is - is it just changing markup & classes? If so, then themes can ship their own theName.html.svelte for most of it, which I do like.

I don't like that it sort of flies in the face of one of Svelte's core tenets, though - of single-file components. There are some use cases in Svelte (we haven't hit them, but some day we might) where there are multiple script tags of differing types, but I suppose that could all still get pulled into the same .js.svelte so probably that doesn't matter.

bnjmnm opened merge request !351

bnjmnm closed merge request !176

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Alright! New MR working_overrides_with_tests. As the name suggests, it includes test coverage!

Here's a bit from the readme explaining how it works

To build the Svelte app using overrides, include the --config_set_theme arg with the theme or module
machine name as the value. If there are multiple themes/modules to include in the build,
provide a comma separated list

`yarn build --config_set_theme=fake_claro`
`yarn build --config_set_theme=fake_claro,some_other_module`

The theme/module root directory must include a /svelte/project_browser directory.
Overriding a specific component requires a name and directory structure match. Ex:
overriding project browsers file: project_browser/sveltejs/src/Project/ActionButton.svelte
requires one of the following in your module
- MODULE_DIR/svelte/project_browser/src/Project/ActionButton.svelte (override the whole component)
- MODULE_DIR/svelte/project_browser/src/Project/ActionButton.css.svelte (override just the <style>)
- MODULE_DIR/svelte/project_browser/src/Project/ActionButton.html.svelte (override just the markup)
- MODULE_DIR/svelte/project_browser/src/Project/ActionButton.js.svelte (override just the <script>)

Overidden Svelte builds are located in a different directory than the default builds:
sveltejs/public/build_custom and sveltejs/public/build respectively.
If overridden CSS or JS is present in the build_custom directory, Drupal will dynamically update the
library and use that instead of the default asset.
effulgentsia’s picture

effulgentsia commented

Very nice to see how far this issue has come already! I haven't reviewed it yet, but just a quick drive by:

I don't like that it sort of flies in the face of one of Svelte's core tenets, though - of single-file components.

My knee-jerk reaction is that I agree with this, and also the mentioned use-case of wanting to override html and css together in the same file, but without overriding the js.

I found https://github.com/homer0/svelte-extend, which seems to have done some thinking in this problem space, and at least superficially, I like the approach of keeping everything in a .svelte file, but having syntax within that file that clarifies what to override vs. what to use from the base component. I wonder if it would make sense to either use that library as-is or to incorporate some of its syntax into our custom approach.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

I'd looked at https://github.com/homer0/svelte-extend in the past and based on my understanding of it (which may of course be wrong), there are two major things that suggest it is not applicable for the theming needs here.

  1. As the example demonstrates, <extend from="./base-component.svelte" html="before" />, extending a component requires knowing the exact location of the base component relative to the extender. This can differ depending on the site, and there's currently no discovery process. That's one of the reasons I went with a naming convention approach & require the over-riders to be specified in the build command
  2. It looks like existing HTML can't change, there can only be additional markup added to the begnning/end

That said, the current MR could function a bit more like this without too much trouble. The single .svelte file is already an option, and it wouldn't require too much additional logic to check which aspect(s) are being overidden and change only those. The extend attribute would not be difficult to add either, and that seems like a very useful feature that would avoid unnecessary duplication. The only part I'm not yet sure about effort-wise is extending JS - svelte-extend appears to dynamically replace anything with matching variable names, but keeps the rest intact. There's an open source example I can reference if necessary, so it seems feasible.

I am partial to having the .css .js .html options, as a means of quickly seeing what parts of the .svelte component are actually being overridden, but it's far away from any hill I'd die on.

chrisfromredfin credited andy-blum.

chrisfromredfin credited lauriii.

chrisfromredfin credited mherchel.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Title: [investigate] Svelte app themability. » [meta] Svelte app themability
Issue summary: View changes

At a discussion at DrupalCon NA 2023 in Pittsburgh, the follow people met to discuss themability and the approaches proposed by Ben Mullins in this issue:

  • Lauri Eskola
  • Tim Plunkett
  • Andy Blum
  • Mike Herchel
  • Chris Wells
  • Ben Mullins

While there, the approaches were discussed, and the following decisions made:

  1. First, refactor many of our values into CSS custom properties. This will allow easy overrides for other themes.
  2. Second, remove the hashed classnames introduced by the Svelte build, so that markup looks more Drupal-y. Using BEM syntax where possible, this aligns better with Drupal markup today. We may need to refactor some of our existing CSS to this end.
  3. Third, allow for the insertion of markup (using Drupal.theme, for example) into component prefixes and suffixes where there's a good opportunity (e.g. the card component).
  4. Fourth, and only if there is great clamoring for it, allow users to override individual Svelte files (or pieces of them), though this introduces a build step. Note: a really motivated person could already today fork and rebuild the front-end to their heart's content.
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs review » Needs work
Issue tags: -Project Browser 2.0 +Project Browser Initiative
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Category: Task » Plan
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

+1 on @chrisfromredfin's comment in #19. I also want to state that there was consensus at the table that "perfect is the enemy of good" and if the insertion of markup into a component prefix/suffix is too time consuming, it should not block beta or stability.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Issue tags: -Project Browser Initiative +core-mvp, +stable blocker

Adding stable-blocking here, but noting from Mike's comment that we will split prefix & suffix out to a non-blocker if it proves too time-consuming.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented

Is this issue still relevant after #3394904: Move styles out of svelte bundle to traditional Drupal library

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

Yes, still relevant. Though moving it out, and that does enhance themability, we still want to convert values to custom properties, do BEM syntax, and ideally #3 if possible, allow prefix/suffix inserting with Drupal.theme().

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented

we still want to convert values to custom properties

Is this necessary since the classes aren't obfuscated, the styles are BEMified, and everything is brought in via the Library API? I think once the plan outlined in #3394904 is done and the styles in PB are reduced to just basic layout styles (similar to the views module when viewed in Stark), admin themes will be responsible for theming the PB UI.

allow prefix/suffix inserting with Drupal.theme()

This probably needs someone investigating if this can be done safely. I imagine Drupal.theme() will break a lot of the svelte event bindings.