RiverLea 1.3.7: code block, api3 select2 legibility & version numbering - #32176
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
| .crm-container code, | ||
| .crm-container kbd, | ||
| .crm-container code { | ||
| display: inline; |
There was a problem hiding this comment.
Do we even need this line? It's the default.
There was a problem hiding this comment.
Maybe. Is there any harm in respecifying to handle places where it's reset to inline?
TBH I think we need to do a ThemeTest section just on code as I didn't realise how many different ways Civi does it. E.g. PrettyPrint (some of which is tackled in 3 & 5 in this PR - #32209).
| border: 1px solid var(--crm-c-background4); | ||
| color: var(--crm-c-text); | ||
| border-radius: 2px; | ||
| padding: 2px; |
There was a problem hiding this comment.
It's pointless setting vertical padding on an inline element. (Vertical padding would apply if we used inline-block).
I'd recommend padding: 0 2px or padding-left: 2px; padding-right: 2px; to be more explicit.
| border-radius: var(--crm-roundness); | ||
| } | ||
| .crm-container pre code { | ||
| display: block; |
There was a problem hiding this comment.
Why this?
Surely the <pre> should have the styling (background, border, padding) and the <code> inside should just not have the bg/brd/padding styling.
I can't think of a case where we have <pre> without an inner <code> and we'd expect it not to be formatted with bg/border etc.?
There was a problem hiding this comment.
Without this it would default to 'inline'. And we want to reset the formatting that's applied in line 338 otherwise you get 2 x borders, padding, etc, no?
There was a problem hiding this comment.
ie this change is doing exactly what you say it should - it's not like this is pre:has(> code)
There was a problem hiding this comment.
Yes we need to reset the bg/border/padding but it's fine to be 'inline' e.g.
There was a problem hiding this comment.
The context where I had to re-add this was in CiviCRM - it looked nothing as clean as your code pen!
But agree some commenting would help here.
| } | ||
| .crm-container p code { | ||
| display: inline-block; | ||
| .crm-container td > code { |
There was a problem hiding this comment.
I believe you that this is necessary, knowing our markup, but a comment as to why would be helpful.
There was a problem hiding this comment.
I explained in a reply to you last week - https://lab.civicrm.org/extensions/riverlea/-/issues/113#note_177664
There was a problem hiding this comment.
apologies for my memory. But my comment was more meant for me/you/other devs in future, who might think: Why is Civi insisting that a code descendant of a table cell should be a block? I mean probably we should PR SearchKit to remove this hack. Or it should go in fixes.css? IDK.
There was a problem hiding this comment.
Yeh fixes.css could definitely be better enforced. It's always that choice tho - separate things that belong together into separate files to be faithful to fixes' function.
|
Oh, I just saw that it's been merged already. Dang. |
3 participants
Overview
Fixes two issues, and updates the version number in the metadata on all of the streams:
Before
API3 select list description
After
API3 select list description
Comments
There's no visual change from reverting
<code>tags to display inline as they should, rather than as blocks, this is more a consequence in extensions.