-
-
Notifications
You must be signed in to change notification settings - Fork 528
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
[3.x] Grid filtering via URL parameters -- Usergroup ACL #16089
[3.x] Grid filtering via URL parameters -- Usergroup ACL #16089
Conversation
Sorry guys; one thing I noticed after opening this was that commit from @Ruslan-Aleev is in here (re template TVs). I'd initially pulled down #16063 as a starting point. I didn't mean for that to be a part of what I was submitting, so just cherry pick the commits I made (4 at this point). @theboxer @Jako @Ibochkarev - Since you guys have been commenting on Ruslan's work on this feature, I added you all to the review list to get this on your radar. |
5cc7234
to
fd33f34
Compare
7fef6aa
to
7b737c8
Compare
7b737c8
to
e9f1104
Compare
e9f1104
to
4f0358d
Compare
4f0358d
to
bbe2b5f
Compare
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.
Thanks for reviewing this @Ruslan-Aleev! I actually have almost all of the grids done and should be able to post a follow-up PR not too long after this one gets implemented. |
@JoshuaLuckers @theboxer @Jako @Ibochkarev - Anyone willing to give this a look? Need one more reviewer to move this along ;-) |
bbe2b5f
to
02b86b9
Compare
02b86b9
to
f3751fc
Compare
Codecov ReportBase: 16.91% // Head: 17.84% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## 3.x #16089 +/- ##
============================================
+ Coverage 16.91% 17.84% +0.92%
+ Complexity 10447 10440 -7
============================================
Files 561 561
Lines 38028 39049 +1021
============================================
+ Hits 6433 6967 +534
- Misses 31595 32082 +487
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Seems to work like expected, but I would love to see it remember the selected "vtab" as well. Now it only works if you select a filter |
That could be done, but I should probably circle back around to that after getting this iteration of the change merged. Aside from the Resource TVs panel, I believe there are only two places where vertical tabs are used (ACLs and during package installation), and the ACLs area is the only place where "bookmarking" the vtab could be useful. |
Fix issue with a non-named area being blank in the list. Also fine-tuned the building of the list array so that duplicate areas show only one list entry along with the correct count of setting entries. Lastly, fixed the sorting of the list to make it based on the label and not the actual value.
f3751fc
to
44ebbd1
Compare
Various tweaks to bring changes from this PR in line with the recently-merged modxcms#16089
A new listener method added in modxcms#16089 was interfering with package installation and upgrades.
Wait, was this an alternative to all of the PRs Ruslan has scheduled for 3.1.0? @smg6511 |
@opengeek - Yes, that's correct. As mentioned in the description above, Ruslan asked if I could create a more global application of this feature. This PR doesn't cover all of the grids, but I have follow up work covering most of the grids (which I'll submit in small batches [or single ones if you like]) that's been waiting for this initial PR's merging. |
This should have been on the 3.1.0 milestone. Ugh. |
A new listener method added in modxcms#16089 was interfering with package installation and upgrades.
* Fix tab tracking error in package manager A new listener method added in #16089 was interfering with package installation and upgrades. * Remove eslint comment I only meant to disable linting on the file while I was working on it. This re-enables it. * grunt build Co-authored-by: Jason Coward <jason@opengeek.com>
If toolbar elements i.e. have to be hidden in a class that extends MODx.grid.SettingsGrid, Ext.getCmp needs an ID to access the toolbar element. In modxcms#16089 the ID was removed. This PR fixes that but with an ID that is set variable with the Ext.id of the current MODx.grid.SettingsGrid. So multiple MODx.grid.SettingsGrid won't interfere with each other because of using the same ID multiple.
What does it do?
This is a global application of the "remembering filter properties" work that @Ruslan-Aleev has been working on. It offers two new methods in the base grid class as well as two new utility methods to handle the the synchronization of grid filtering properties between the grid store and the URL; the feature additionally tracks the main and nested tabs via a listener on the base tabs class.
A couple other small changes to note:
GetAreas
class so that it outputs a unique, alpha ordered (case insensitive) list with the correct settings counts. The "None" label now shows up as well (previously the entry for an unspecified area was checked for via null instead of empty, which is why only the count currently shows).Why is it needed?
@Ruslan-Aleev requested my help a while back to make his feature more globally-applicable and to also bring in the ability to remember the correct tab. Utilizing class inheritance, this approach also will cut down on a lot of code for the filterable grids overall as, aside from a couple special cases where one filter depends on the value of another (such as grids with namespace and area filters), all filter application and clearing methods within individual grids can be removed.
How to test
The current set of changes covers the ACL Usergroup editing area (which is the most complex, having the nested vertical tabs in the Access Permissions tab).
Related issue(s)/PR(s)
Relates to all of the "remembering filter properties" PRs.