Update FC Set grids - Correct group headers config - #16951
Conversation
Remove custom groupTextTpl and make minor tweaks to grid column groupability and Lexicon
Ibochkarev
left a comment
Ibochkarev
left a comment
There was a problem hiding this comment.
Nice direction overall — dropping the custom groupTextTpl and riding GridBase’s singleText / pluralText path is the right fix for #16948. That’s the code-judo version of the issue (option 1b), not another encodeQuotes copy-paste.
One blocker before this lands: groupBy: 'tab' on FCSetFields. The old groupBy: 'tab_rank' kept groups in region-rank order; the custom tpl only existed to show the tab id in the header. Grouping by the tab string sorts alphabetically by id, so regions can reorder relative to the form. Please keep groupBy: 'tab_rank' and put a groupRenderer on the tab_rank column that returns record.data.tab. Then you still get the base tpl + encodeQuotes, and hideGroupedColumn keeps behaving as before.
Visibility groupRenderer and groupable: false on the high-cardinality columns look good. Lexicon split into abbr/long with the old-key aliases is fine for EN/Crowdin.
Happy to approve once the Fields grouping order is restored.
| autoHeight: true, | ||
| grouping: true, | ||
| groupBy: 'tab_rank', | ||
| groupBy: 'tab', |
There was a problem hiding this comment.
This is the one I’d change. groupBy: 'tab' fixes the header text but loses rank ordering.
Keep groupBy: 'tab_rank', and on the tab_rank column add something like:
groupRenderer: function(v, unused, record) {
return record.data.tab;
}Base groupTextTpl will then show the tab id via {text}, singleText/pluralText still apply, and groups stay in form order. Also means hideGroupedColumn keeps hiding tab_rank instead of the Region (ID) column.
| singleText: _('field'), | ||
| pluralText: _('fields'), |
There was a problem hiding this comment.
Good call setting these. Without them GridBase falls back to record/records, which is what these grids have effectively been showing since the custom groupTextTpl wasn’t wired into groupingConfig.
| groupRenderer: function(v) { | ||
| return v ? _('yes') : _('no'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This is the right Ext pattern for boolean grouping. Same on the TVs grid below.
| @@ -600,3 +599,9 @@ | |||
| $_lang['tv_description_desc'] = 'Usage information for this TV shown next to its caption in Resource editing pages and as a tooltip in the Elements tree.'; | |||
| $_lang['tv_elements'] = 'Input Option Values'; | |||
| $_lang['tv_elements_short_desc'] = 'Defines the selectable options for this TV, which may be manually entered or built with a one-line <a href="https://docs.modx.com/current/en/building-sites/elements/template-variables/bindings/select-binding" target="_blank">database query</a>.'; | |||
|
|
|||
| // Temporarily match old keys to new ones to ensure compatibility | |||
| $_lang['clear_cache_on_save_msg'] = $_lang['clear_cache_on_save_desc']; | |||
| $_lang['rrg_drag'] = $_lang['resource_groups_panel_desc']; | |||
| $_lang['tv'] = $_lang['tv_abbr']; | |||
| $_lang['tvs'] = $_lang['tvs_long']; | |||
There was a problem hiding this comment.
EN-only + aliases looks right for this repo (Crowdin owns the rest). Minor nit: tvs_abbr isn’t referenced anywhere yet — fine if you want the set complete, otherwise it can wait until something needs it.
3 participants
What changed and why
groupTextTplfor two grid configs, deferring to the already-established base configAside from minor enhancements, the change is to guard against future problems as noted in the related issue.
How to test
tvs_longvalue toTemplate's Variables(adding a single quote as shown or double quote if you wish, with or without backslash escape), refresh the cache and ensure the Template Variables grid (3rd tab) still displays as expected. Note that this text is used in each group's header, e.g., “Category: None (2 Template Variables)” or, in the case of the first tab's grid, “Region (ID): modx-page-settings-box-left (6 Fields)” if changing thefieldorfieldsLexicontvs_longand add an escaped single quote in the value, again clearing cache and verifying grid operation. I add this as the custom db entries might already be encoded (I didn't look into this, but I wouldn't be surprised if they are)Related issue(s)/PR(s)
Resolves #16948
Compatibility notes
n/a
Breaking change assessment
n/a
Test coverage
n/a for Extjs
Contributors
@opengeek
AI tool use
n/a