Riverlea - add Stream entity - #32127
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
(Just a note, if testing locally on a pre-existing site you will need to uninstall and then reinstall Riverlea in order to create the DB table) |
|
I'm not in a position (skills-wise) to comment on the code changes here - I can't comment on other consequences they might have. I can only really look at this from a product perspective: Firstly a previewer is definitely really nice - ThemeTest would benefit from that (I spend quantifiable time moving back and forth from Display Setting). But this is more than that… At present:
With this proposal:
And the benefit of all of this change is the previewer? Or Something else ("paves the way for future UI to a) pick a stream based on what it looks like on your site" - ie in a realtime preview?). Side point: as RiverLea flows towards the big C of Civi Core - maybe it's name makes less and less sense? It's no longer a theme like Greenwich and more Civi's theme engine. So maybe - at least if it becomes an entity in core, its name should be something generic like CRMThemer, or at least more generic like 'river' (as river -> stream, is helpful naming to emphasise the cascade order of css). |
|
Hey @vingle With this approach I see two ways to make new streams:
The only advantage implemented in this PR is the previewer. But it's a fairly small step from here to:
|
So all of a stream's custom css (variables, darkmode versions) would be stored within the Entity? If I store it in Git, someone else changes a bunch of variables, can I get that back into my system? Presumably any part of the entity can be edited?
That'd be cool. I created an empty SubTheme extension at the end of last year which I think still works - I just made it public: https://github.com/vingle/branch with a Stream called 'Albany'. Might need a tiny bit more testing/docs before becomming a template. I used the name 'branch' as that seems like a subsidiary water-system from a stream. But obvs branches have other meanings on git so perhaps another name works (I thought about 'estuary', 'tributary', 'source', 'channel', 'canal', 'brook' - branch seems the most accurate, but probably confusing).
That's interesting - maybe overlaps with the oembed stuff… ie for the example of someone creating a 'join newsletter' or 'petition' widget they want to embed on a dozen different sites, each with a different font and bg-colour… then giving each embed some unique class could let you save a unique variable… ie |
The standard way managed entities work is you a file version (which can be checked in to git). Then you have a database (database) version. These start in sync. If you make edits to the database version (using the UI), you then have the option to a) export them - ie make the same changes to the file version; b) revert the database changes, go back to the file version. If you make edits to the file version they will generally be synced to your database version (unless that has local changes - which you might not want to lose. In that case you have the option to revert to the file version to get the latest changes from the file.)
https://lab.civicrm.org/ufundo/river_rea/-/tree/dev . I called it river_rea which is obviously way too close to River Lea 🤦
My thinking was that we don't need to create another hierarchy, and we should use stream for all the variations within River Lea. A stream = River Lea Theme = super easy to maintain theme. (I've started seeing it the other way round, the stream is the little drops of ink you add to the river at the beginning, and then the river takes that and spreads it all over the UI :))
I think this approach would lend itself to adding a url parameter ie. https://mycivi.com/contribution-page?stream=my_blue_stream and https://mycivi.com/contribution-page?stream=my_green_stream which then loads the appropriate vars on that page (without having to add css classes programmatically, which would be hard I think). |
|
My thinking was that we don't need to create another hierarchy, and we should use stream for all the variations within River Lea. A stream = River Lea Theme = super easy to maintain theme.
Fair point.
(I've started seeing it the other way round, the stream is the little drops of ink you add to the river at the beginning, and then the river takes that and spreads it all over the UI :))
Nooooh! 😅 as the ‘crm-c-text’ variable / drop flows from the River source, it first takes one colour from core, then as it branches into one Stream it takes a different colour, while in a second Stream no new variable/drop is defined so it stays the same (ie the order has to follow the cascade..)
(without having to add css classes programmatically, which would be hard I think).
That makes sense
|
|
Further possibility with this kind of model: automating contrast checks between vars we know need to contrast. IE messages in the editor like "your crm-c-primary and crm-c-primary-text currently have contrast rating 5:1 which meets AA but you should increase for AAA" |
Sold! |
|
https://lab.civicrm.org/extensions/deptfordcreek/-/merge_requests/1 demonstrates the change in what "external streams" would look like before/after the core change. The "external stream" could also put all it's var in the Managed file, and not have any separate css files. |
|
I think we need thel change to |
d6bfa48 to
26d4898
Compare
ufundo
changed the title
|
I've narrowed this down to just the entity plumbing. Listing, Previewing + Editing are separated to #32344 |
|
retest this please |
26d4898 to
dee729c
Compare
|
@ufundo some test failures here... |
89fbaa6 to
9cdac76
Compare
|
tests should pass now @colemanw ( a static/non-static function got dropped in rebasing somehow 😬 ) |
| */ | ||
| class CRM_riverlea_Upgrader extends \CRM_Extension_Upgrader_Base { | ||
|
|
||
| // By convention, functions that look like "function upgrade_NNNN()" are |
There was a problem hiding this comment.
@ufundo Looks like an upgrader is needed to add the new sql table (and all this boilerplate clutter can be deleted too).
I recently updated the docs about how to do so: https://docs.civicrm.org/dev/en/latest/step-by-step/create-entity/#5b-add-upgrade-step-for-extensions-used-in-production
There was a problem hiding this comment.
ah thanks that's what I was after!
| 'input_type' => 'Select', | ||
| 'description' => ts('Extension that provides this stream.'), | ||
| 'default' => NULL, | ||
| // @todo why not work? |
There was a problem hiding this comment.
I think because the civicrm_extension table is slightly odd.
There was a problem hiding this comment.
I think it's kind of immaterial for now... for the time being this will always be provided by a *.mgd.php and it won't make sense to edit it.
There was a problem hiding this comment.
@ufundo which part doesn't work? The FK index in SQL? Or the join in Api4?
If the former, it's now possible to declare an EntityRef without the FK index like so:
civicrm-core/schema/Core/UserJob.entityType.php
Lines 158 to 164 in d1a8138
There was a problem hiding this comment.
This still doesn't work in SearchKit / Formbuilder. I can't a) join Stream records to Extension records, b) in-place edit that field, c) add an afform filter.
I think maybe it's because file is not the primary key on civicrm_extension ? But we need to use a machine name (rather than id) because this is typically set in a managed record ( https://github.com/civicrm/civicrm-core/pull/32127/files#r2104616568 ).
I think for now it is fine to not have the entity_reference for now?
| 'label' => E::ts('Hackney Brook'), | ||
| 'description' => 'named after the Hackney Brook, a tributary of the River Lea that ran through Finsbury Park', | ||
| 'is_reserved' => TRUE, | ||
| 'extension' => 'riverlea', |
There was a problem hiding this comment.
where extension is set
8be0500 to
fbac906
Compare
fbac906 to
e9b567e
Compare
|
Did a more thorough pass of the README. One change (maybe controversial) is to remove use of "subtheme" as I think it a source of confusion. It suggests that the streams are subthemes of the Riverlea theme. But for a user, there is no theme they can select called Riverlea. Riverlea is an extension, and each of the streams is a theme that you can select as your site theme. I think Stream = Riverlea Theme |
Looks great - thanks! Just two comments above.
The architecture before was using Civi's subtheme functions - but I'm assuming this PR ends that? I was going to suggest mentioning sub-themes once at the start to help people, but it's true there's no loadable RiverLea theme so maybe it's better they use the new term. |
On the topic of sub-thingies ... does this let us create a "sub-stream": a stream that inherits from another stream and applies overrides? This is different to modifying a full clone to create a new stream.
There could be 2 versions of this:
|
That's what the goal is with the Customiser (still a draft), forking an existing core stream (a brook?!). I don't think that's available with this PR tho - right @ufundo? Streams created as new entities via the API will still inherit directly from Minetta? Or can that be changed to other Streams with this? But - I agree with the principle should be to only store in a new stream/sub-stream the minimum that's changed. The customiser UI will also only expose a limited number of variables - one of the roadmap jobs with RL is how to simplify it to allow fewer variables to have more impact. Most people don't want to understand how five different background colours work with each other across Civi; that's RL implementing Greenwich+Shoreditch faithfully; most people expect to have one bg colour that cascades to other background regions with tints automatically. |
|
Good questions.
It still uses the same
So at the moment I have been working on the copy-and-edit approach. There's no direct support for this. I actually started with a
Notwithstanding the above:
100% this. |
With the customiser, you can create a blank stream (which will then inherit from core, and look like Minetta), or you can copy an existing stream (Walbrook or Hackney or one you made) and then tweak it. |
|
Just remembering @vingle 's presentation on Riverlea... To put it in those terms, I see it as:
By all means start from an existing wallpaper design when coming up with a new one. But we shouldn't get into the business of layering different wallpapers with semi-transparent bits on top of each other. |
Added a note to the intro of Customising to say it's early days (as this applies to adding vars to parent theme too). I also added a snippet for if you want to override one or two vars directly (this is an alternate approach that may work in the short term for some simple use cases) |
|
One use case I can see for inheritance: I have my "org theme" which has 20 vars set. But then I want to do a dozen "campaign themes" which use 19 vars but set the primary colour differently. If I want to make a change to the button-roundness across the board, how? I think it's relevantly different that I am the maintainer of the parent and child themes... they are a cluster. But it would be nice to avoid having to change that button-roundness var on every one of my dozen campaign themes. One way this can be handled with the structure in this PR is:
That way all streams use the same css file source, but can tweak at the entity level. You can update the css file once and it will affect all the campaign streams. In this sense there's a bit of inheritance built in within each stream, which can be leveraged for this sort of thing. |
|
Further drawback of inheritance... you end up with versioned dependency issues. I'm hoping that people will share nice streams they have made for others to use. This is much easier if the stream definition is self-contained -- NOT "my |
| Exploring the _variables.css file will give you idea of how much can be overwritten. | ||
| ### 2. Add a custom CSS snippet in an extension | ||
|
|
||
| You can override variables by listening to `hook_civicrm_alterBundle`: |
There was a problem hiding this comment.
Hmm,
As this isn't something I've ever done (this is an existing function, not something you added with this pr, right?)… if you add a snippet in an extension - how does someone else find out that an extension has added a css snippet making all my buttons purple, and how do they turn it off?
I'm happy to tell themers that they can stick :root {--crm-c-primary: green; } in a theme css file and it will apply front end or backend if they want a quick fix. If it breaks other things it's quick to find where it's loading via an Inspector.
But I can picture extension devs potentially doing unpredictable things here to make their own extension look like they want it (increase padding between buttons, e.g.). Inspector won't reveal where this comes from - how many themers are going to be comfortable (or know that's what they should do) debugging an odd colour / spacing issue via API explorer? They'd just be forced to turn off RiverLea.
There was a problem hiding this comment.
Yes this is existing functionality, and it's a bit quick and dirty.
I was imagining it for my_client_extension rather than adding to my_feature_extension.
TBH you could do it in my_feature_extension if you are careful about scoping, ie something like:
.my-feature-container {
--crm-button-spacing: 1rem;
}
Maybe adding a health warning here or an example like the above would help? We can't stop extension authors enqueing buggy css, so I think "here's some advice" is better than nothing.
There was a problem hiding this comment.
We can't stop extension authors enqueing buggy css, so I think "here's some advice" is better than nothing.
But we can also not advise them how to do it - and just remove this new bit?
Otherwise we're just saying 'there's this other thing you can do, but don't do it!'.
(my concern is that most people who agencies give the job of fixing css are looking for files to edit - not the dbse.)
Perhaps there could be info that an advantage of CSS variables for extension developers is you can make small extension specific changes without writing css but by changing the variables within the scope of your extension. E.g.
<div class="a-large-button">
<button>Name</button
</div>
<style>
.a-large-button {
--crm-btn-padding-inline: 1rem;
--crm-btn-height: 3rem;
}
</style>
But this is more of a docs discussion…
|
Really good points here @ufundo. 100% agree if the tldr is 'lets try to avoid sub-streams unless the sub-stream and stream are from the same dev. I think the question has a front-end / back-end divide more than anything. Back-end - most people I'm guessing will settle on a stream, with minor tweaks, unless they're doing something like Nook or One Click and want their own unique UI (or have unique accessibility needs like @aydun's current Finsbury Park fork - which I guess could be a sub-stream of Hackney). But front-end, I could picture an agency who tends to build front-end sites using Framework Z (hopefully not a really css framework). Their Civi pages need to integrate so they make a stream that does 90% of the integration work which they ship with all their Civis. But every site they build has it's own design so they need to sub-theme their stream. Still I can see why that creates a cascade of risks, literally - so perhaps we either shout 'only do this at your own risk' or we leave this for a future development - at least within the customiser. I guess it would just mean you would treat a sub-stream as a core stream in the Customiser UI? |
I think if its a frontend theme used outside of Civi, then that could integrate with Riverlea using vars etc WITHOUT being a stream (more along the lines of option 1). It's a more manual integration... but if you are a shop building something more bespoke then I think that's reasonable. But I don't think streams themselves should be over-engineered for this kind of use case (and I think you could probably make it work in a way that allows you to overlay tweaks using streams - i.e. a combo of Option 1 and 3).
There's an |
|
Jenkins test this please |
|
Looks to me like discussion has simmered down on this PR and we're at a point of fine-tuning that could be addressed in followups. I say we merge this & continue those discussions via chat or gitlab or followup PRs. |
colemanw
added
the
merge ready
|
Shall we merge? That gives us some |
|
I'm guessing this fatal error is linked to this? [14-Jul-2025 14:41:15 Europe/London] Uncaught PHP Exception Error: "Class "CRM_riverlea_DAO_RiverleaStream" not found" at ~/Sites/localhost/dru-civi/vendor/civicrm/civicrm-core/Civi/Api4/Generic/AbstractEntity.php line 101 It won't let me access Update: deleting templates_c fixed it. |
4 participants
Overview
Move the Stream definitions in Riverlea from hard-coded to a new entity.
This enables a "Previewer" which allows you to switch streams locally, without changing the site settings.
Paves the way for future UI to a) pick a stream based on what it looks like on your site; b) editing and creating custom streams through the UI (AKA "Customiser").
Before
riverlea.phpAfter
CRM.riverlea.previewer()to open a previewer widget, which allows you to switch between streams on the flycustom_css, which allows for easy adding fixes. these are then cached via assetbuilder (essentially using the stream name and last modified as key) which overcomes the performance concerns with previous attempt at Customiservarsdefined using php, which could then be validated/updated automatically etcScreencast.from.2025-02-18.14-27-34.mp4
(note all previewer changes are on the frontend and local to your current session - the site theme setting isn't touched. It does require starting with a Riverlea theme selected)
Work in progress
Need to add upgrader to create the new table on existing installs. I think this goes in the core upgrade step for core extensions?
Would be good to get some feedback on how it's working / overall schema.
Known Limitation
civicrm.cssoverride in Thames and Walbrook (it can only switch the var values on the fly). At this stage it's a dev tool, so I think it's reasonable. (I also think the overrides in Thames and Walbrook don't need to be incivicrm.css- they could be moved into the stream specific css file)river.csswill only track changes to the entity, but if you usecss_fileand modify the file it won't necessarily know. Again, I think this is reasonable. It's not expected to edit css files directly in production - and you (will soon) be able to editcustom_csson the entity if you need to.Technical Details
<civi-riverlea-previewer>. I'm not sure how we feel about these but I think it's very nice for a self contained component that can use the API and talk to the browser.