Riverlea - merge crm-c-light-text and crm-c-text-light

[ufundo]

updated description

Overview

Merge eerily similar css vars, then try to fix some things that break as a result of the merge.

This standardises on crm-text-light. It also updates use of crm-text-dark so that crm-text is set to either text-light or text-dark depending on the context/dark mode.

(Previously some of the streams set text-light to something dark in dark mode, but this a) caused some issues; b) made it hard to follow what was going on in dark mode.

There's also some consequences for:

• crm-heading-col and friends
• crm-checkbox-list-bg and friends
• crm-c-yellow and friends
• alert-text-warning and alert-background-warning

I've done my best to follow through the consequences as minimally but logically as poss.

Comments

I noticed this when looking at the Standalone login page on Hackney Brook with Dark Mode -- the alert message text color seems to be undefined, because it's using crm-c-light-text , which isnt defined in that context.
`

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[vingle]

@ufundo thanks for spotting the crm-c-text-light and crm-c-light-text bug. I probably would have merged them the other way as a) I count 54 'light-text' vs 16 'text-light'. Also, generally the naming convention (tho it's not brilliantly consistent as it wasn't figured out in advance) is adjective-noun ie warning-background or success-border or light-text. That said because it's not consistent I won't stand in the way of text-light.

The other main change here around warning colours will need some testing. Emphasis colours are a headache because there's so many different ways they're used and there always needs to be good colour contrast. E.g. for warning you get:

• alerts, with one colour for bg, one for border one for text
• popup alerts, where the background is defined by the streams and in the current streams is white (like Shoreditch) or black (like Greenwich). But these still include emphasis colours on the border and the alert icon, and these colours need to contrast with the choice of bg
• buttons, eg btn-cancel which may also have an icon, like a trash or a times/cross, which might also have a colour, and yet the colour for those icons shouldn't display in the buttons if the buttons have a bg (but should if they don't as in Hackney/Finsbury)
• Searchkit also adds emphasis colours for table cells and dropdown items and these use different bootstrap classnames. Dropdowns themselves have a Stream-wide bg colour, so if you have a delete link in a dropdown, it also needs to have a good contrast ratio.
  (The darkmode flips for all of these different combos adds an extra pain point).

In short it should be a case of 'this is the red or green I want for danger/success' but in reality those colours need to work with the context and each stream has their own quirks. This is one of those problems I only realised while building RiverLea so it's ended up quite complex, and yet also not without issues. I tried to track this issue here: https://lab.civicrm.org/extensions/riverlea/-/issues/29 but as recently as v1.3 I had to add more emphasis colour variables to get the right contrast ratios, and I still occasionally find bugs, ie https://lab.civicrm.org/extensions/riverlea/-/issues/38, because of some combination, say, of an alert and an icon.

All of which is to say a) I will test these alert colour changes in a bunch of contexts next week; b) if it doesn't break anything else - great - if it does, then c) maybe it would be good to have a call to see if there's a way to rationalise/simplify this all. Most sub-theme makers won't need this level of complexity/configurability (in the core variables file, there's 9 variables with 'success' in the name and 11 with 'warning' - almost all specifying colours!).

[ufundo]

hi @vingle thanks yeh I pulled a thread and... it kept going.

Great if you can test a bit. Maybe a call would be good. I was thinking about streams and customised screens and how it would be really great if we can get as clearer view as poss of which variable pairs might appear as text-on-background or vv. But appreciate its not simple! I think the notification text disappearing in dark mode is a new example of the same bug

For naming, sorry I didnt notice that convention. I thought text-light made sense in a parent-variant kind of way? I was thinking it might help devs to be able to search crm-c-text and get

• crm-c-text
• crm-c-text-light
• crm-c-text-dark
  ...

[vingle]

I was thinking it might help devs to be able to search crm-c-text and get…

That makes sense. Flip side is being able to search crm-success and get crm-success-text, crm-success-border, crm-success-bg. As I say tho, I've not got a strong feeling on that.

FWIW - this screengrab doesn't occur for me, is it because of you forcing black on text in dark-mode standalone because of the login screen (#31991)?

[ufundo]

FWIW - this screengrab doesn't occur for me, is it because of you forcing black on text in dark-mode standalone because of the login screen (#31991)?

No, I can recreate on smaster today. It's only for Hackney Brook.

(the login screen change is tightly scoped to the login screen)

[ufundo]

Flip side is being able to search crm-success and get crm-success-text, crm-success-border, crm-success-bg

Right yeh that does make sense. Maybe even more important to see these together if success-text and success-bg are going to be one of these need-contrast pairs...

[ufundo]

Thinking about it I think those are slightly different.

I.e. we have:

...success-text
...success-bg

...warning-text
...warning-bg

which are little grouped things and make sense that way round.

But I think "light" and "dark" work slightly differently. I don't think these groups make sense:

...light-green
...light-blue
...light-text
...light-bg

...dark-green
...dark-blue
...dark-text
...dark-bg

These seem the more logical groups to me maybe?

...green-light
...green
...green-dark


...blue-light
...blue-dark

...text-light
...text-dark

...bg-light
...bg-dark

[vingle]

These seem the more logical groups to me maybe?

That's true. There's probably a whole bunch of changes that could be done with naming of variables because it was begun without much of a plan for them.

I'm also conscious that once people have built their own streams/themes from these variables it will be much harder to change them.....

[vingle]

I've fixed the HackneyBrook DarkMode bug here: #32077 on 6.0beta (in a slightly more direct way).

Maybe this PR could be just about name-changes, rather than the other two bundled changes of fixing Hackney DM notifications and changing the way alert colours are names/specificed in dark-mode?

The alert changes I think will need quite a bit of testing, because of the various places alerts are used and the various things that appear in alerts. Your changes look simple but make me nervous about a bunch of known unknowns.

If the main motivation is "my brain couldn't cope with e.g. --crm-c-light-green now being dark green and vice versa" then these changes are about making life easier for theming devs and might need some discussion so my brain can cope too!

But the other two issues - hackneybrook darkmode notification and text-light/light-text mixup feel like they could/should be merged quickly..

[vingle]

This change will impact Thames, which sets --crm-alert-background-help: var(--crm-c-blue-light);

[ufundo]

Yeh. I think what would be good here is a separate semantic label that is like "--crm-c-enabled".

Then the default for help and enabled colours can both be green.

If Thames wants to make help things blue, it can do without making enabled things blue. (And no one has to set green to blue)

[ufundo]

Maybe this PR could be just about name-changes, rather than the other two bundled changes of fixing Hackney DM notifications and changing the way alert colours are names/specificed in dark-mode?

Totally agree. I think I originally thought the notifications issue was caused by fixing the light-text dark-text thing, and so it spiralled. But lemme try again a bit more narrowly.

If the main motivation is "my brain couldn't cope with e.g. --crm-c-light-green now being dark green and vice versa" then these changes are about making life easier for theming devs and might need some discussion so my brain can cope too!

Yes. The motivation is:
a) make is simpler for devs to work with
b) try to consolidate the structure a bit, so it doesn't feel terrifying to make changes to colours, because there's too many permutations and combinations to think about. We're trying to get to a place where users can pick some colours and be reasonably confident it will work... and so I'm trying to find steps in that direction. Indeed, it would be good to know that the streams should all "just work" because they are variations of the sort of thing users will be able to vary -- it feels like the core and stream css has got a bit tangled maybe?

[ufundo]

Have updated this to just merge those two variable names.

But fixing that does create a problem in Hackney Brook dark mode - --crm-c-primary and --crm-c-primary-text which get used together both end up mid grey.

This doesn't happen before because --crm-c-primary-text is set to --crm-c-light-text which is undefined. I think this is the thread I started pulling.

I'm adding the smallest fix I can think, which is to set --crm-c-primary-text to explicit white. But the reason I was keen to pull the thread, is that this feels like adding more paint at the stream layer to fix holes. Whereas I would really like to be simplifying the streams, and making the core do the work.

[vingle]

That sounds a good suggestion

[vingle]

Indeed, it would be good to know that the streams should all "just work" because they are variations of the sort of thing users will be able to vary -- it feels like the core and stream css has got a bit tangled maybe?

I’m not sure where you specifically refer to, but the lines between core hard set css property (eg enabled extensions are a green variable), core set variable (eg green is hex value Y) and stream override (eg this stream uses a different shade of green) is so far opinionated and definitely open for discussion & improvement.. sometimes I will have good reason for doing it some way (usually it’s a variable if Shoreditch and Greenwich do the thing differently) other times it’s arbitrary, probably better ways to do it with more brains. That said - changing names of variables is something we should aim to do before there are custom streams in the wild…..

[ufundo]

@vingle I think the dark mode --crm-c-primary and --crm-c-primary-text are working now.

Also the specific issue of the alert message text on the login box is a non-issue now #31965 is merged.

Do you think this could be merged now?

[ufundo]

retest this please Jenkins

[vingle]

Yes, that’d be good - then can move onto other variable name cleanups.

[ufundo]

Hmm. There's an issue for Minetta with checkbox-list item. It's another place where dark mode is flipping a text var dark=>light but the background stays light.

I think the best fix involves fixing a value for --crm-c-text-dark. I'm trying that in version I'm pushing now.

[ufundo]

It's a bit bigger, because essentially I'm updating places where --crm-c-text has been used on a light background (like yellow) to use --crm-c-text-dark. This is the same in light mode, but in dark mode when --crm-c-text becomes light, these places maintain a dark text.

[vingle]

It's a bit bigger, because essentially I'm updating places where --crm-c-text has been used on a light background (like yellow) to use --crm-c-text-dark.

That's a big string you're pulling - maybe you shouldn't do that until we speak later on Thurs? I can see what you're aiming for, but you're changing quite a bit of internal logic by flipping crm-c-text and crm-c-dark-text (which was only intended to help clarify that a light bg wants a dark text foreground). It's barely used as a variable, and maybe could be dropped - but here you're elevating it to be used a lot more..

It feels like you're adding a new complexity to re-rationalise it differently, but mostly just to solve a bug here? You're also adding new variable names? Maybe structural proposals like new variable names or reworking how specific variables work could start on Gitlab as a discussion, not in the middle of a growing PR which I thought was just to clean up the light-text text-light bug?

(Apologies if I misunderstood - am just post-moving house, am ill, and today is my childcare day. I should have more time to read/test slowly tomorrow)

[ufundo]

I have another PR incoming which I hope might (amongst other things) help make testing easier: #32127

[ufundo]

I tried to pull back on the crm-c-text-dark changes to keep this minimal, but on discussion with @vingle think it is good to include => so reinstated now.

[vingle]

I've done quite comprehensive tests on Minetta light + dark* and I've found a bunch of bugs, mostly around dark-mode contrast issues, but none are from this PR - they are in RiverLea itself.

So while I will keep testing across the other three streams I would recommend to merge this now, and I will make a new PR with fixes for these newly found dark-mode legibility bugs, and do a final round of testing.

*by comprensive I mean - all the ThemeTest screens, the Civi dashboard (all dashlets), Contact dashboard (all tabs), FormBuilder UI (all tabs), SearchKit UI (all tabs) and API4 UI - in case we ever want to try and automate visual tests with something like BackstopJS (ref: discussion)

[ufundo]

@colemanw are you happy to merge based on @vingle testing?

[vingle]

While continuing to check this in Walbrook and Hackney Brook I did find two issues linked to this PR. I'm putting fixes as part of RL 1.3.8 - which is a bit of an epic fix of darkmode issues, the vast majority not linked to this, but just emerged via deeper testing. So either this PR needs to be merged first before that, or wait with this and just merge 1.3.8 instead, coming shortly…

[ufundo]

Closing, superceded by @vingle 's version