Fix: Make search results and tree items display in consistent manner - #16958
Fix: Make search results and tree items display in consistent manner#16958smg6511 wants to merge 2 commits into
Conversation
Adds status classes to items to make search result display consistent with the tree
Code quality fixes
smg6511
changed the title
|
@Ibochkarev @mkschell - Hey guys, can you check this out in the near future? Thx ;-) |
Hi! I'll take a look today. |
Ibochkarev
left a comment
Ibochkarev
left a comment
There was a problem hiding this comment.
Hey Jim — thanks for picking this up. Matching search results to the tree is the right problem.
I looked at how Resource\GetNodes / Element\GetNodes already build cls and iconCls, and I think we can get much closer to the tree with less new surface area. Right now Search.php ships a nested attributes bag and searchbar.js re-derives CSS from it. That second presentation path will drift the next time someone tweaks tree status styling.
Ask: have the processor emit ready-to-use cls (and a resolved icon) via modResource::getStatusClasses() / getIconClasses(), and follow the same conventions Element\GetNodes already uses (element-node-disabled, static via icon). Then the XTemplate can print {cls} and we can drop getStatusClasses plus most of the attributes envelope.
Line notes below. One more thing: _uberbar.scss does not yet mirror _tree.scss for unpublished / hidemenu / deleted, so a lot of the new status classes barely show even when JS attaches them. Either bring those tokens/class names into this PR, or narrow the claim to icons/folder and leave status styling for the follow-up.
Happy to chat if that framing feels off.
| $attributes = [ | ||
| 'isFolder' => $record->get('isfolder'), | ||
| 'isStatic' => $record->get('class_key') === modStaticResource::class, | ||
| 'status' => [ | ||
| 'published' => $record->get('published'), | ||
| 'deleted' => $record->get('deleted'), | ||
| 'hidemenu' => $record->get('hidemenu') | ||
| ] | ||
| ]; |
There was a problem hiding this comment.
Could we lean on $record->getStatusClasses() here instead of rebuilding published / deleted / hidemenu by hand?
Same idea for icons via getIconClasses() — that path already covers folder semantics (childrenCount > 0 || isfolder), content-type icons, and the mgr_tree_icon_* settings the tree uses.
Rough shape:
'cls' => implode(' ', $record->getStatusClasses()),
'icon' => /* normalize prefix from getIconClasses() */,Then the client does not need this attributes bag for resources.
| /** @var modElement $record */ | ||
| foreach ($collection as $record) { | ||
| $this->results[] = [ | ||
| $attributes = [ | ||
| 'isElement' => true, | ||
| 'isStatic' => $record->get('static'), | ||
| 'status' => [ | ||
| 'disabled' => $record->get('disabled') ?? false |
There was a problem hiding this comment.
disabled only really lives on plugins. Element\GetNodes gates it with $elementClassKey === modPlugin::class and emits element-node-disabled.
Calling get('disabled') on chunks / snippets / tvs / templates papers over that boundary. Worth matching GetNodes here (and using the same class name), plus treating static the way the tree does — via icon-file-code-o — instead of a .static status class the tree never paints.
| getStatusClasses: function(values) { | ||
| const status = values?.attributes?.status || null; | ||
| let classes = ''; | ||
| if (status) { | ||
| switch (values.type) { | ||
| case 'resources': | ||
| classes += !status.published ? ' unpublished' : '' ; | ||
| classes += status.hidemenu ? ' hidemenu' : '' ; | ||
| break; | ||
| case 'plugins': | ||
| case 'users': | ||
| classes += status.disabled ? ' disabled' : '' ; | ||
| break; | ||
| // no default | ||
| } | ||
| classes += status.deleted ? ' deleted' : '' ; | ||
| classes += values?.attributes?.isStatic ? ' static' : '' ; | ||
| } | ||
| return classes; | ||
| }, |
There was a problem hiding this comment.
If the processor hands back a ready cls string, this helper can go away entirely.
As written it also invents class names the tree does not use (disabled vs element-node-disabled, and .static as a status class). That is how search and tree drift apart even when both "have status info".
| switch (values.class) { | ||
| case 'MODX\\Revolution\\modDocument': | ||
| return 'file'; | ||
| return values.attributes.isFolder ? 'folder' : 'file' ; |
There was a problem hiding this comment.
Tree folder icons come from getIconClasses() (parent-resource when there are children or isfolder). Hardcoding folder only when isfolder is set will diverge when auto_isfolder is off or a container already has kids.
If icon resolution moves server-side, this branch disappears with it.
|
|
||
| &.disabled, | ||
| &.unpublished { | ||
| em { | ||
| opacity: 1; | ||
| } | ||
| } | ||
|
|
||
| &.disabled { | ||
| color: $unpublished; | ||
| i.icon { | ||
| color: $unpublished; | ||
| } | ||
| } | ||
|
|
||
| &.deleted { | ||
| a { | ||
| text-decoration: $delTextDeco; | ||
| } | ||
| } |
There was a problem hiding this comment.
Tree styles for these states live in _tree.scss ($unpublished, $hidden, $delTextColor, element-node-disabled).
Here .unpublished only bumps em opacity, .hidemenu gets no rules, and .deleted skips the color token. If the goal is visual parity with the tree, I would reuse those tokens / class names in this PR — or keep this PR to icons/folder and leave status styling for the CSS follow-up you mentioned.
2 participants
What changed and why
Added status information to search result data and a new method to the front end XTemplate to match how the tree displays the various states of Resources and Elements. Some specific updates of interest:
Design Note
I only touched css minimally here, but will propose a number of style enhancements in a separate PR.
How to test
Test in an environment with many Resources and Elements, having various published/deleted/disabled states and compare the search display to the tree display. The two should match.
Note that some Extras override the tree icon (e.g., Collections, Gallery), but do not override the search one; that disconnect would need to be addressed by the relevant Extras’ maintainers.
Example Screenshots
On the left are a couple clips of my tree; on the right, a few search result examples showing various states. You can spot where specific ones match up.
Related issue(s)/PR(s)
This is a different approach than and supersedes the proposed change in #16659.
Compatibility notes
n/a
Breaking change assessment
None
Test coverage
n/a
Contributors
n/a
AI tool use
n/a