Render and mange admin pages tabs server side #5855
Conversation
marcospri
commented
Nov 23, 2023
marcospri
commented
- Create one route and template per tab
- A macro renders the tab headers in each new template
- Create one route and template per tab - A macro renders the tab headers in each new template
marcospri
force-pushed
the
ssr-org-tabs
branch
from
7dd6392 to
128cbf8
Compare
| @@ -161,6 +161,7 @@ def includeme(config): # pylint:disable=too-many-statements | |||
| config.add_route("admin.role.override.delete", "/admin/role/overrides/{id_}/delete") | |||
|
|
|||
| config.add_route("admin.organization", "/admin/org/{id_}") | |||
| config.add_route("admin.organization.danger", "/admin/org/{id_}/danger") | |||
There was a problem hiding this comment.
New route/url per tab. /usage already existed.
| @@ -37,23 +37,6 @@ | |||
| integrity="sha512-IOebNkvA/HZjMM7MxL0NYeLYEalloZ8ckak+NDtOViP7oiYzG5vn6WVXyrJDiJPhl4yRdmNAG49iuLmhkUdVsQ==" | |||
| crossorigin="anonymous" | |||
| referrerpolicy="no-referrer"></script> | |||
| <script src="https://cdn.jsdelivr.net/npm/@vizuaalog/[email protected]/dist/bulma.min.js"></script> | |||
There was a problem hiding this comment.
Removing all the stuff added to manage them JS side.
I reckon it was interacting wierdly with this PR.
Of course if we went this route we'd need to adapt the AI pages as well.
| @@ -201,3 +201,33 @@ | |||
| </div> | |||
| {% endcall %} | |||
| {% endmacro %} | |||
| {% macro tab_headers(request, tabs, active) %} | |||
There was a problem hiding this comment.
This is probably the less nice part of this PR.
Having one route render one tab is pretty easy but this deals mostly with setting the "is-active" in the right tab.
| @@ -0,0 +1,3 @@ | |||
| {% extends "lms:templates/admin/base.html.jinja2" %} | |||
| {% block header %}Organization {{ org.id }}{% endblock %} | |||
There was a problem hiding this comment.
Initially I had the tab headers rendered here but now it's each tab that calls the macro.
This base template doesn't add much right now but it useful to have it.
| {% import "lms:templates/admin/macros.html.jinja2" as macros %} | ||
| {% extends "lms:templates/admin/organization/base.html.jinja2" %} | ||
| {% block content %} | ||
| {{ macros.organization_tabs(request, org, "admin.organization.danger") }} |
There was a problem hiding this comment.
Render the headers, set the active one using the route name 🤷
| request_method="GET", | ||
| renderer="lms:templates/admin/organization/danger.html.jinja2", | ||
| ) | ||
| def danger(self): # pragma: no cover |
There was a problem hiding this comment.
We could create a little abstraction around this type of view. Not for this PR.
There was a problem hiding this comment.
The biggest mental block about this is the fact that it currently requires a lot of refactoring. My previous PRs (#5842 and #5845) were going in the direction of getting something working with as fewest changes as possible.
However, if we are honest with ourselves, the LMS admin is not a SPA, and adding pieces of a SPA to it is a bit of a hack. From my point of view, the proper way to have routing in the admin is this, using good ol' server-side template partials and macros.
They can get a bit messy, but they are straightforward and easy to understand when you have to add more routes and look at existing code to follow the same pattern.
With the other PRs it was easier to forget something. With this one, it won't work until you get it right because there's less magic involved.
|
Going for the FE approach for now, let's revisit if that gets complicated. |
