-
-
Notifications
You must be signed in to change notification settings - Fork 529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add next/prev/up buttons to the resource panel #15464
base: 3.x
Are you sure you want to change the base?
Conversation
That issue was about adding prev/next links to the documentation, not to MODX 😆 This looks cool as well though! |
this is feature, not bug =) Sorry, don't know how to work with Docs |
Co-authored-by: Joshua Lückers <[email protected]>
Co-authored-by: Joshua Lückers <[email protected]>
Co-authored-by: Joshua Lückers <[email protected]>
Co-authored-by: Joshua Lückers <[email protected]>
{ | ||
$arr = []; | ||
|
||
$q = $this->xpdo->newQuery(modResource::class, ['parent' => $this->parent]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like this approach. On large sites, this basically means that everytime you open resource to edit you'll fetch all resources under the same parent, which could be hundreds or even thousands of records in some cases, just to get 2 specific IDs.
Wouldn't be better to query directly for the resource under and above the current one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was idea for modx docs, not for modx CMS) my mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one way to get the prev/next without having to get all resources: add a where
that matches the sortby.
For the next:
$qNext->where([
'id:!=' => $this->get('id'),
'AND:menuindex:<=' => $this->get('menuindex'),
]);
$qNext->limit(1);
$qNext->sortby('menuindex', 'ASC');
For the previous:
$qPrevious->where([
'id:!=' => $this->get('id'),
'AND:menuindex:>=' => $this->get('menuindex'),
]);
$qPrevious->limit(1);
$qPrevious->sortby('menuindex', 'ASC');
The "equals or larger/smaller" makes sure that items with the same menuindex still appear.
It means two queries, but will be much more optimized than loading all siblings, especially on large sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to use more optimized queries.
Even if the initial request wasn't meant for the core, this could be a nifty feature to add to 3.1. Maybe there's something to be said for optimising the UI a bit more, to make it take up less space in an already busy toolbar? Perhaps a buttongroup instead of multiple buttons? Not, sure, just ideas. |
Even if just the keyboard actions were implemented and there were no buttons added this would be a handy feature. That said, the suggestion of a button group would make sense to keep the UI as simple as possible in this area. @wax100 - Would you like someone else to pick up where you left off on this PR? |
Is moving to the next/prev resource really something that's used that much on all resources, that it deserves the spot in the already packed action bar? I can see this useful in some specific scenarios for resources under specific parent, where it may be handy to quickly move to the next/prev one. But in general space, I don't think this is needed. |
What does it do?
Add buttons next \ prev \ up to resources
Why is it needed?
Enhancement #12Related issue(s)/PR(s)
Enhancement #12 modxorg/DocsApp#12