Render listing buttons as components #11939
Conversation
laymonage
added
type:Cleanup/Optimisation
status:Needs Review
component:Universal listings
Manage this branch in SquashTest this branch here: https://laymonagerender-listing-button-4hyuw.squash.io |
laymonage
force-pushed
the
render-listing-button-as-component
branch
from
bc12865
to
284e9c4
Compare
Instead of manually rendering them via the parent dropdown component and a custom template, delegate each button's rendering to itself by making use of the default Button template added in 2274a44 and the {% component %} tag. This allows the buttons inside the dropdown to have their own markup, e.g. to use a <button> element instead of <a>. Remove the `button button-small button-secondary` CSS classes from the base ListingButton class. We'll assume instances of this class is rendered inside a dropdown, and hence do not need the extra CSS. If people still do need it, advise them to add it to the classname themselves.
laymonage
force-pushed
the
render-listing-button-as-component
branch
from
284e9c4
to
a467e71
Compare
| @@ -541,7 +542,7 @@ def bulk_action_choices(context, app_label, model_name): | |||
| next_url = context["request"].path | |||
|
|
|||
| bulk_action_buttons = [ | |||
| PageListingButton( | |||
| ListingButton( | |||
There was a problem hiding this comment.
The bulk action buttons are not page specific, so they really shouldn't use PageListingButton.
There was a problem hiding this comment.
Good shout on the bulk actions @lb-. Looking at this again this probably should even be a Button instead of ListingButton (now that the two classes no longer have any implementation difference in this PR).
In the future we could probably accept a bulk action to define its own button instance (or at least the class) and set it on the bulk action button class. Then, here we can just use something like if getattr(action, "button", None) and fall back to this default button otherwise. That way developers can have complete control over the button's template, and can add additional elements/attributes as necessary.
| <form action="{{ button.url }}" method="post"> | ||
| <input type="hidden" name="user_pk" value="{{ user_pk }}"> | ||
| <button type="submit" class="{{ button.classname }}" {{ button.base_attrs_string }}> | ||
| {% icon name=button.icon_name %} | ||
| {{ button.label }} | ||
| </button> | ||
| </form> |
There was a problem hiding this comment.
Per the example in #11915, minus the csrf_token since we haven't tackled the problem of passing the request object yet. That is the step 2 described in #11915 (comment).
| def __init__(self, label="", url=None, classname="", **kwargs): | ||
| classname = f"{classname} button button-small button-secondary".strip() | ||
| super().__init__(label=label, url=url, classname=classname, **kwargs) |
There was a problem hiding this comment.
I spent a lot of time thinking about the best way to go around this, but I see a few problems.
We previously managed to support leaving these CSS classes in place because the listing dropdowns are not rendered as components, and instead rendered directly via _dropdown_items.html, which leaves out the classname. Unfortunately this means it's impossible for the buttons to have their own markup, e.g. to use a <button> inside a <form> as described in #11915, which broke after the same code for the dropdown was reused for the users listing view.
If we refactor the listing dropdown items to be rendered as components, we now have the problem of these classnames showing up, which breaks the styling inside the dropdown.
The problem is that we use the same PageListingButton (which extends ListingButton and thus has the classnames) for both the top-level listing button ( register_page_listing_buttons) and the button inside the "more" dropdown ( register_page_listing_more_buttons). Our documentation example for register_page_listing_more_buttons uses the plain Button class, but in core itself we use PageListingButton (which was refactored to even more specific classes in c05c2cc).
We technically can separate the two, e.g. by having PageListingButton behave as-is, and having another class e.g. PageListingMoreButton that does not have the classnames. But then we'll have to update all our default buttons to use PageListingMoreButton as the base instead of PageListingButton. I feel this isn't worth the trouble, and we would be better off just dropping the default class names and advising people to either move towards register_page_listing_more_buttons or supply the classname themselves.
Happy to discuss this further though!
|
Just saying I think this is a nice approach, I haven't run the code locally though but a bit more flexibility but also avoiding hard-coding links as buttons is helpful. This may also lead to a pattern we can use for #11770 - currently we have no way to easily make the button component render a |
For the first step described in #11915 (comment).
Instead of manually rendering them via the parent dropdown component and a custom template, delegate each button's rendering to itself by making use of the default
Buttontemplate added in 2274a44 and the{% component %}tag.This allows the buttons inside the dropdown to have their own markup, e.g. to use a
<button>element inside a<form>instead of<a>.Remove the
button button-small button-secondaryCSS classes from the baseListingButtonclass. We'll assume instances of this class is rendered inside a dropdown, and hence do not need the extra CSS. If people still do need it, advise them to add it to theclassnamethemselves.