Allow an object's __str__() or other method to be used as the card header

[ababic]

For a bit of background: I'm playing with the idea of using this for wagtailmenus. The most meaningful thing to use as a card header for MenuItem instances is a method that returns a page's title (if the item links to a page), or custom menu text value if that is provided.

I would be happy with card headers being blank until the form was saved (or maybe just something generic, like 'New menu item'), and also with the card headers not changing dynamically during editing.

I'd be happy to have a crack at this myself in PR (though, I may need some help on the React side of things) but wanted to first ask if you think this is something you would want to support, and whether you might have any ideas about how best to approach the problem?

Thank you for the great work so far!

[ababic]

I've thought of a way we might actually be able to support dynamic updating with this approach:

If we could gather the form values for a specific Inline and ajax post them (along with a ContentType ID, and the name of the desired method/property method/attribute) to a view that attempts to assemble (but not save) an instance and call the desired method on it... That could be reused for pretty much any model. Triggering that process could prove tricky though (listening for changes on any field change would be overkill). I imagine you might also have to swap the csrf_token out for a new each time, but that's no biggie.

[unexceptable]

@ababic You made any progress with this? I'm willing to help as needed because using this for wagtailmenus would make my life much nicer than the current gui.

Will also dig into #6 and see if I can help there.

[ababic]

Hi @Adrian-Turjak. I'm afraid all I did was play around so far - trying to familiarise myself with the bits and pieces. I'm normally not the type to raise things and not tackle them myself, but I'm completely unfamiliar with React, so my confidence fizzled out pretty quickly.

I would honestly settle for card titles on items not dynamically updating using this approach. I think it would be fine to just have:

• obj.__str__() as the card title for any existing items
• "New {model_versbose_name}" as the card title for any that haven't yet been saved

I feel the coloured status highlighting would provide enough feedback to users for them to keep track of which item is which - Dynamically updating the title on every change (or perhaps changes to one or more specified fields) would just be a nice touch, but not crucial.

Being able to specify different methods for the string would be useful, but not necessary. I suppose it might be useful to look for a get_condensedinlinepanel_card_title method (kinda like Wagtail has get_admin_display_title), and use that if it exists (falling back to __str__ as a default)?

I'd also like to suggest that maybe this is the default behaviour if you didn't specify card_header_from_field, card_header_from_js or card_header_from_js_safe - only with those would the dynamic updating kick in.

Keen to hear your thoughts :)

[unexceptable]

@ababic Sorry for not responding right away!

Non-dynamic is more than enough for my use cases, and for menus would be fine. New menus you'd be keeping open anyway and then on reloading/saving the titles would now be needed.

Although if we aren't dynamically updating titles, we should have the js alter the title to something like Edited {model_versbose_name} when changed rather than keep the old title since that may be confusing. This isn't a requirement, but would be useful and a bit of a compromise between dynamic and not.

As for how to do the dynamic stuff, card_header_from_field might be enough for most use cases. When you define the panel in the view and specify that param, it gets passed in the context and the panel js is setup to dynamically update the title with whatever content is in the input for that field. I've not played with React, nor am I particularly good at JS, but that kind of stuff is fairly common from what I've seen.

card_header_from_js might be a good way to do it for more complex things where there isn't a clear field that could be a title (like menu items, because of page titles). Maybe if all you'd need to do is supply a js function that provides a title, and then in the panel js code, if that function is defined, call it to refresh the title on card close.

Again, my JS is rusty so I don't know what the best method to do here is, but the approach where you pass in some JS to provide the title for the card isn't a terrible idea. Since for something like the menu objects, when you select a page to link to, the page 'title' is somewhere in the DOM, and if you know where, writing some js to pull that out isn't impossible. Or if the link text field has content, use that instead.

Any of that make sense? :P

[ababic]

Thanks @Adrian-Turjak! some good points there.

Although if we aren't dynamically updating titles, we should have the js alter the title to something like Edited {model_versbose_name} when changed rather than keep the old title since that may be confusing

My personal feeling is that the current behaviour of colour-highlighting is enough to provide feedback that something has been edited - if the item has not been saved yet, to me, it would still make sense for it to have a "New menu item" heading. Changing to "Edited menu item" could confuse things if you added new items to an existing menu.

card_header_from_field might be enough for most use cases. When you define the panel in the view and specify that param, it gets passed in the context and the panel js is setup to dynamically update the title with whatever content is in the input for that field.

Yes, I believe this is the current behaviour. It's a perfectly good solution for cases when the card header does track a specific field.

card_header_from_js might be a good way to do it for more complex things where there isn't a clear field that could be a title (like menu items, because of page titles)

This is a potential route, but I'm not really sure how this option works currently, or how I'd go about using it to generate menu item card headers, especially since we'd need to access field values for selected pages in order to report the same things as str() (see https://github.com/rkhleics/wagtailmenus/blob/master/wagtailmenus/models/menuitems.py#L84) That would have to involve some other view to access that data from the Page.

When you select a page to link to, the page 'title' is somewhere in the DOM, and if you know where, writing some js to pull that out isn't impossible. Or if the link text field has content, use that instead.

Yes, but it's not always the 'title' field that is used from pages, it can be customised using the WAGTAILMENUS_PAGE_FIELD_FOR_MENU_ITEM_TEXT setting. I think our only option is to involve some other view that can provide this data to the JS. The menu item models can also be substituted for custom models that may well override the __str__ and menu_text methods (hence why, assembling an object and calling the model method would be preferable)

To have card headers update dynamically for menu items, you'd need to be able to trigger both when link_text or link_page was changed - and that would probably require new options to support. That's a lot to expect, so why I'd prefer to keep the solution simple / not dynamic for the time being.

If we can call __str__ for each existing object when loading the form, and keep new card headers as "New menu item", I don't feel like that would require any complex JS changes at all, but would produce a better default experience than card headers being blank. Do you or @kaedroho have any insight into whether calling __str__ on an object when the panel is loaded would be possible?

[unexceptable]

My personal feeling is that the current behaviour of colour-highlighting is enough to provide feedback that something has been edited - if the item has not been saved yet, to me, it would still make sense for it to have a "New menu item" heading. Changing to "Edited menu item" could confuse things if you added new items to an existing menu.

I realise the way I phrased that made little sense! I was mostly thinking for the header from __str__ case, as an old title in that case may be confusing since it wouldn't be able to reload. Otherwise a menu item called: "My Github Page" on load, which you edit to "My Facebook Page" will still have the old title until you save, despite you condensing that item, and then going to edit another one. The colour change is probably enough to be honest, but in larger lists might be confusing.

card_header_from_js might be a good way to do it for more complex things where there isn't a clear field that could be a title (like menu items, because of page titles)

This is a potential route, but I'm not really sure how this option works currently, or how I'd go about using it to generate menu item card headers, especially since we'd need to access field values for selected pages in order to report the same things as str() (see https://github.com/rkhleics/wagtailmenus/blob/master/wagtailmenus/models/menuitems.py#L84) That would have to involve some other view to access that data from the Page.

It appears to just be an eval() which means you can easily throw in some javascript that pulls the data from the formset in the same way that the from field dynamic refresh does it.

Yes, but it's not always the 'title' field that is used from pages, it can be customised using the WAGTAILMENUS_PAGE_FIELD_FOR_MENU_ITEM_TEXT setting. I think our only option is to involve some other view that can provide this data to the JS. The menu item models can also be substituted for custom models that may well override the str and menu_text methods (hence why, assembling an object and calling the model method would be preferable)
I'd say this is too complicated for the comparative worth of the end result and would make wagtailmenus harder to maintain. :(

To have card headers update dynamically for menu items, you'd need to be able to trigger both when link_text or link_page was changed - and that would probably require new options to support. That's a lot to expect, so why I'd prefer to keep the solution simple / not dynamic for the time being.

Currently the dynamic card header refresh happens on card close, which should work fine. Given how the js is setup for this, you should be able to easily enough throw in an eval as it is that checks the formset for both of those values and outputs a result on card close. The formset is built here, and that will be accessible in the js as a json. The problem is the case where some has used WAGTAILMENUS_PAGE_FIELD_FOR_MENU_ITEM_TEXT since you'd need some way to add that field here.

If we can call str for each existing object when loading the form, and keep new card headers as "New menu item", I don't feel like that would require any complex JS changes at all, but would produce a better default experience than card headers being blank. Do you or @kaedroho have any insight into whether calling str on an object when the panel is loaded would be possible?

Adding that could likely be done with just another value in the extra dict added in get_form_extra_data once you have the object here, and then you replace this case here to grab the obj_str value out of form.extra .

[ababic]

I was mostly thinking for the header from str case, as an old title in that case may be confusing since it wouldn't be able to reload. Otherwise a menu item called: "My Github Page" on load, which you edit to "My Facebook Page" will still have the old title until you save, despite you condensing that item, and then going to edit another one. The colour change is probably enough to be honest, but in larger lists might be confusing.

Thank you for clarifying. That may be fine. I guess it's one of those things that you can't judge until you see it working.

Currently the dynamic card header refresh happens on card close, which should work fine. Given how the js is setup for this, you should be able to easily enough throw in an eval as it is that checks the formset for both of those values and outputs a result on card close.

Great. That sounds like the way to go then, if @kaedroho feels like this is something they want to support?. If not, I suppose we can implement that side of things within wagtailmenus itself, but do feel like it would make a useful addition.

Adding that could likely be done with just another value in the extra dict added in get_form_extra_data once you have the object here, and then you replace this case here to grab the obj_str value out of form.extra

Thanks. I did try to figure out where to set this last night, but I was way off :)

[unexceptable]

Currently the dynamic card header refresh happens on card close, which should work fine. Given how the js is setup for this, you should be able to easily enough throw in an eval as it is that checks the formset for both of those values and outputs a result on card close.

Great. That sounds like the way to go then, if @kaedroho feels like this is something they want to support?. If not, I suppose we can implement that side of things within wagtailmenus itself, but do feel like it would make a useful addition.

Oh, there shouldn't need to be anything special that needs to change here. Looking at it, we can probably just use it as is with the following javascript passed in to card_header_from_js:

(function(){
    if (form.extra.hasOwnProperty('link_page')){
        page_title = form.extra['link_page']['title'];
        if (page_title){
            return page_title;
        }
    } else if (form.fields.hasOwnProperty('link_text')){
        link_text = form.fields['link_text'];
        if (link_text){
            return link_text;
        }
    }
    return "";
})();

Then in python that would look like:

header_from_js = """
    (function(){
        if (form.extra.hasOwnProperty('link_page')){
            page_title = form.extra['link_page']['title'];
            if (page_title){
                return page_title;
            }
        } else if (form.fields.hasOwnProperty('link_text')){
            link_text = form.fields['link_text'];
            if (link_text){
                return link_text;
            }
        }
        return "";
    })();
"""

# .....

        panels = (
            CondensedInlinePanel(
                app_settings.MAIN_MENU_ITEMS_RELATED_NAME, label=_("menu items")
                card_header_from_js=" ".join(header_from_js.split()),
            ),
        )   

Note the " ".join(header_from_js.split()) to properly remove the new lines and somewhat compress the js to fit into the eval better. That way in the wagtailmenu code we can have the js in an editable format, but we can pass it to the constructor nicely.

It's the WAGTAILMENUS_PAGE_FIELD_FOR_MENU_ITEM_TEXT case where we'd need some changes to condensesinlinepanel that would allow us to configure what additional fields are pulled into the extra dict here. Then we can update the js for wagtailmenus to use that field instead.

[ababic]

Thanks @Adrian-Turjak,

Let's take wagtailmenus-specific solutions out of the equation for the time being, as we can work that out later (this board isn't really the place for that anyway).

What I want to iron out here specifically, is whether @kaedroho would be interested in supporting the following as default behaviour?

• Card titles are generated using obj.__str__() for existing objects
• Card titles for new items would follow the format New {{ lower_case_model_verbose_name }}
• If card_header_from_js or card_header_from_field were used, they would do what they do now and replace the default card title on load, and when the card is collapsed / opened.
• If neither of card_header_from_js or card_header_from_field were used, no dynamic updating of the card title would take place (they would just remain the same until the form was saved).

I feel like that would create a better 'out-of-the-box' experience for most use cases (familiar behaviour too - most 'inline' solutions I've seen for the django admin behave this way)

[unexceptable]

@ababic check out #16 and tell me how that works for you. While handling other default cases I ended up sneaking in card header from obj.__str__() :P

[ababic]

Thanks @Adrian-Turjak. I should have some time this weekend to try things out.

[unexceptable]

@ababic with #16 now merged you want to close this issue?

And I guess once the next version is published to pypi I should potentially look forward to a branch of wagtailmenus to test. ;)

[ababic]

Yeah, let's close this :) the rest of the stuff here is wagtailmenus specific, so we'll work that out in that project :)