DEV: Change admin site settings filter to be route based #33167
Conversation
21b7efa to
c1eb0dd
Compare
f241e92 to
93cd89c
Compare
93cd89c to
193dd0e
Compare
| siteSettings: categories[n], | ||
| }; | ||
| }); | ||
| static async findAll(params = {}) { |
There was a problem hiding this comment.
The only change here is to add async/await. Unfortunately that touches all lines.
| this.router.replaceWith( | ||
| "adminSiteSettingsCategory", | ||
| this.controllerFor("adminSiteSettings").get("visibleSiteSettings")[0] | ||
| .nameKey | ||
| ); |
There was a problem hiding this comment.
No need to be so fancy here. This just evaluates to "required".
| if (!controller.get("visibleSiteSettings")) { | ||
| controller.set("visibleSiteSettings", siteSettings); | ||
| async model(params) { | ||
| if (!this._siteSettings) { |
There was a problem hiding this comment.
We could consider clearing this when exiting the site settings area so it has to be refetched, but I'm not sure if that'd serve any use TBH.
|
Looks good to me. I also played with that on my local and didn't notice any regression 🎊 |
app/assets/javascripts/admin/addon/routes/admin-site-settings.js
Outdated
Show resolved
Hide resolved
| filteredContent(category) { | ||
| return category ? category.siteSettings : []; | ||
| } | ||
| @alias("model") filteredSiteSettings; |
There was a problem hiding this comment.
do we need alias each time? (this one and the ones in app/assets/javascripts/admin/addon/controllers/admin-site-settings.js). I don't like alias it makes things super hard to debug.
There was a problem hiding this comment.
Is it the decorator that interferes with debugging? I.e. would manually defining a getter be okay?
What is this change?
The filtering is currently being done in the controller. This leads to weird things where the filter loop might already be initiated and we get double filtering, as well as the issue that we can't abort like we could with a route transition.
How does it work?
modelin the route.