Label/header customisation and better defaults #16

unexceptable · 2017-08-15T05:33:15Z

Deals with #15

Needs to be compiled and tested, but this should achieve what we want, or be close to it.

unexceptable · 2017-08-15T05:44:19Z

@kaedroho I can't seem to get this to compile. I'm not much of a frontend developer and I haven't touched typescript before, so I may have made some mistakes.

Error is:

condensedinlinepanel/static/condensedinlinepanel/src/condensedinlinepanel.tsx(67,37): error TS2322: Type '{ forms: Form[]; panelLabel: string; renderCardHeader: renderCardHeaderFn; canEdit: true; canDele...' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<CardSetProps, ComponentState>> & { child...'.
  Type '{ forms: Form[]; panelLabel: string; renderCardHeader: renderCardHeaderFn; canEdit: true; canDele...' is not assignable to type 'CardSetProps'.
    Types of property 'formTemplate' are incompatible.
      Type 'string | undefined' is not assignable to type 'string'.
        Type 'undefined' is not assignable to type 'string'.
node_modules/@types/react-dnd/index.d.ts(83,70): error TS2314: Generic type 'Component<P, S>' requires 2 type argument(s).
node_modules/@types/react-dnd/index.d.ts(84,69): error TS2314: Generic type 'Component<P, S>' requires 2 type argument(s).
node_modules/@types/react-dnd/index.d.ts(129,66): error TS2314: Generic type 'Component<P, S>' requires 2 type argument(s).
node_modules/@types/react-dnd/index.d.ts(130,67): error TS2314: Generic type 'Component<P, S>' requires 2 type argument(s).
node_modules/@types/react/index.d.ts(165,11): error TS2559: Type 'Component<P, S>' has no properties in common with type 'ComponentLifecycle<P, S>'.

Which doesn't look related to my change though.

Can you have a look? All I'm trying to do is pass through the label value to the right place.

kaedroho · 2017-08-15T08:37:41Z

This compiled for me, what version of Typescript do you have installed? (i'm using 2.1.4)

kaedroho · 2017-08-15T08:39:32Z

condensedinlinepanel/static/condensedinlinepanel/src/components/CardSet.tsx

@@ -178,7 +179,7 @@ export class CardSet extends React.Component<CardSetProps, {}> {
                e.preventDefault();
                return false;
            };
-            addButton = <button className="condensed-inline-panel__top-add-button button bicolor icon icon-plus" type="button" onClick={onClickAddButton}>Add</button>;
+            addButton = <button className="condensed-inline-panel__top-add-button button bicolor icon icon-plus" type="button" onClick={onClickAddButton}>Add {this.props.panelLabel}</button>;


I'm thinking it might be nicer to make the panelLabel argument replace the whole label, including "Add". This would be useful for people working in languages that might put the word for "Add" after the label.

Works for me. I was mainly mimicking what inline panel did, but this also solves my problem.

We should then put Add as the default where the string constant is set.

unexceptable · 2017-08-15T23:38:07Z

and looks like my version of typescript is 2.4.2

I downgraded to your version, and that fared somewhat better, but for some reason the webpack bundle had a ton of random little changes.

unexceptable · 2017-08-16T04:57:52Z

Ok, have played with this a bit. Gotten it to work. Missed some bits, and have decided to also tweak things a little.

Silly thing that Inline panel does is use the label for both the ADD button and the Heading. Which is weird, but whatever. I've added a heading value to this panel, but have it default to label is not set, so that it doesn't break existing usage. I'll propose a patch to fix that in core wagtail for the inlinepanel because it makes sense there too provided it still works the same until the use the heading value itself.

Will submit a new patch. I've gotten it to compile myself, but my version of webpack is different, so it adds a lot of random stuff which is unrelated. Better for you to just recompile it and test that it makes sense.

unexceptable · 2017-08-16T05:40:01Z

Ok, cool. That should do it! Please compile and test yourself, and see if that works for you. :)

kaedroho · 2017-08-18T08:35:43Z

condensedinlinepanel/edit_handlers.py

@@ -176,7 +177,8 @@ def bind_to_model(self, model):
            'relation_name': self.relation_name,
            'related': related,
            'panels': self.panels,
-            'heading': self.label,


~~I think we should leave the heading field called label so it's the same as Wagtail's builtin InlinePanel.~~

EDIT: Scratch that. What you suggested in your last comment sounds good to me

kaedroho · 2017-08-18T08:37:22Z

condensedinlinepanel/edit_handlers.py

@@ -176,7 +177,8 @@ def bind_to_model(self, model):
            'relation_name': self.relation_name,
            'related': related,
            'panels': self.panels,
-            'heading': self.label,
+            'heading': self.heading,
+            'label': self.label,


I think it would be really nice is the add label defaulted to "add {{ model name }}", so it works the same as InlinePanel

Will add that in tomorrow (hopefully).

kaedroho · 2017-08-18T08:42:30Z

Thanks @Adrian-Turjak!

I've gotten it to compile myself, but my version of webpack is different, so it adds a lot of random stuff which is unrelated. Better for you to just recompile it and test that it makes sense.

Yeah, this is rather annoying. I've noticed it happens when two people are using different OSes (I'm using Linux).

unexceptable · 2017-08-31T02:53:37Z

So i'm looking at having the default be "Add {model name}" but...

I can't seem to see how if at all InlinePanel actually does this. And from testing it, I don't think it does. If I don't set a label it actually doesn't even seem to have the header bar:

InlinePanel(
     'featured_casestudies',
     label='Featured Casestudies'
),

InlinePanel(
    'featured_casestudies',
    # label='Featured Casestudies'
),

I'll add this to the condensed version, and potentially also to the core one since that does appear odd.

@kaedroho Can you a double check I'm not going mad and that InlinePanel does indeed not have a sensible default if no label is set?

* label now defaults to "Add {model name}" * heading defaults to "{model name}" * card heading now defaults to __str__() for the related model rather than an empty string, and the heading for new cards will be "New {model name}" until the page is saved. * When card_header_from_field is used, new cards will at first have "New {model name}" as the heading until the card is closed and updated. * Default new card heading of "New {model name}" will not be set for card_header_from_js as it is unsafe to assume status of 'value' var and it is better to allow a default to be set in the user defined js.

unexceptable · 2017-08-31T04:24:11Z

latest commit handles default cases a little better, and also deals with #5 while I'm at it.

I think we're pretty much done.

ababic

Hi @Adrian-Turjak. Thanks for putting this together. As promised, I had time to check things out. It's very nearly there. I just have a few recommendations for where things might be improved.

ababic · 2017-09-02T21:55:13Z

condensedinlinepanel/edit_handlers.py

@@ -171,12 +174,17 @@ def bind_to_model(self, model):
        else:
            related = getattr(model, self.relation_name).related

+        related_name = camelcase_to_underscore(


The labelling of Django models is conventionally configured by setting verbose_name and verbose_name_plural strings on a model's Meta class. It's quite common for those strings to be marked for translation too (as they are in wagtailmenus) to adjust labelling appropriately for different languages. So, I think we need to be making better use of those here.

related.related_model._meta.verbose_name_plural.capitalize() would be a better fit for a default heading. I suggest using capitalize() instead of title() too, as it doesn't make sense to mess with a developer's specified labelling any more than necessary (this is more in keeping with treatment of verbose names within the rest of Wagtail also).

And _("New {model_verbose_name}").format(model_verbose_name=related.related_model._meta.verbose_name) would be a better choice the new item card header (with no case manipulation at all).

Yes! This is much better. I just blanked on this and didn't think it through. Will follow up with that change.

ababic · 2017-09-02T21:58:22Z

condensedinlinepanel/edit_handlers.py

@@ -154,9 +156,10 @@ def required_formsets(cls):


 class CondensedInlinePanel(object):
-    def __init__(self, relation_name, panels=None, label='', help_text='', min_num=None, max_num=None, card_header_from_field=None, card_header_from_js=None, card_header_from_js_safe=None):
+    def __init__(self, relation_name, panels=None, heading='', label='', help_text='', min_num=None, max_num=None, card_header_from_field=None, card_header_from_js=None, card_header_from_js_safe=None):


Since we are allowing heading and label to be set independently, for completeness, should we add a new argument to allow the new item header text to be set independently too? Maybe as new_card_header_text? I think this could be quite useful (For example, I would probably use this in wagtailmenus to set this text to just "New item", because with the heading and add button both mentioning 'menu items', repeating that again in card headers seems a little unnecessary).

I don't see why not. It's easy and can just be one extra arg most people can ignore if not needed.

* switch to using verbose_name * add basic translation support for these values * add new_card_header_text arg to panel for an easy user set override.

unexceptable · 2017-09-04T01:54:09Z

@ababic Back to you. :)

And when/if you use this for wagtailmenus, I will be more than happy to review and test!

ababic

Looking very nice indeed! Just a few more tiny things.

ababic · 2017-09-04T21:44:04Z

condensedinlinepanel/edit_handlers.py

-            related.related_model.__name__).replace("_", " ").title()
+        heading = self.heading or _(related.related_model._meta.verbose_name_plural.capitalize())
+        new_card_header = self.new_card_header_text or _("New %s" % related.related_model._meta.verbose_name)
+        label = self.label or _('Add %s' % related.related_model._meta.verbose_name.capitalize())


I would say that capitalising verbose_name for label here probably isn't necessary.

It might not be priority for the project at this point in time, but using named parameter substitution for translatable strings helps to generate much nicer source files for translators to work from.

Will drop the capitalising.

Also will redo the translations. I actually haven't played with translations much and didn't actually realise that in your examples you were calling .format after the _ call. I've just had a quick read through the django docs and realised that with their example:

output = _('Today is %(month)s %(day)s.') % {'month': m, 'day': d}

I'll update with a new commit.

Although because I find it less verbose and easier to read, I tend to stick with old style string formatting unless I explicitly need new style features, so apologies in advance for that. They can take it away from me when they deprecate it (which probably won't happen). :P

Yes, I must admit I still find myself using both in different places. I have a mild preference for format() recently though. Happy with this 😀

ababic · 2017-09-04T22:09:37Z

condensedinlinepanel/edit_handlers.py

@@ -156,7 +156,7 @@ def required_formsets(cls):


 class CondensedInlinePanel(object):
-    def __init__(self, relation_name, panels=None, heading='', label='', help_text='', min_num=None, max_num=None, card_header_from_field=None, card_header_from_js=None, card_header_from_js_safe=None):
+    def __init__(self, relation_name, panels=None, heading='', label='', help_text='', min_num=None, max_num=None, card_header_from_field=None, card_header_from_js=None, card_header_from_js_safe=None, new_card_header_text=""):
        self.relation_name = relation_name
        self.panels = panels
        self.heading = heading or label


I'm not sure using label for heading here under these conditions is very intuitive now that we define a more sensible default for heading lower down. I think we're just doing this for the sake of backwards compatibility, but I think it would be better instead to decouple the two completely and include some guidance in the release notes. Any thoughts?

While I think this panel type isn't being used widely, I tend to lean towards backwards compatibility for any changes unless entirely unavoidable.

Plus, a lot of what I've ended up doing here, even with the defaults we've now added, I actually want to move to the standard wagtail inline panel, so keeping the changes here, and there in sync so the two are pretty much drop in replacements for each other (apart from card_header args) is useful. I've already started with: wagtail/wagtail#3765

I'm happy with that. Might be a good idea to mark it with a TODO comment or something though, to make that clear. Maybe somethings like:

# TODO: label is used below for backwards compatibility. We may want to rethink this later. self.heading = heading or label

unexceptable · 2017-09-05T23:10:36Z

@kaedroho I think we're good! @ababic seems happy, and it seems to work. Test yourself to make sure I haven't broken anything, but it's with you now. :)

I'll also start doing some follow pull requests to wagtail core for the inline panel itself to port some of these defaults and such over.

kaedroho · 2017-09-07T08:47:55Z

condensedinlinepanel/templates/condensedinlinepanel/condensedinlinepanel.js

            {% endif %}

            if (value==null){
                var value = "";
+                var needsEscaping = false;


No need for var here

kaedroho · 2017-09-07T08:48:51Z

condensedinlinepanel/templates/condensedinlinepanel/condensedinlinepanel.js

        renderCardHeader: function(form) {
            {% if self.card_header_from_field %}
                {# Get the card header value from a field and escape it #}
                var value = form.fields['{{ self.card_header_from_field|escapejs }}'];
+                if (value===null){
+                  var value = "{{ self.new_card_header }}";


No need for var here too

kaedroho · 2017-09-07T08:52:38Z

condensedinlinepanel/templates/condensedinlinepanel/condensedinlinepanel.js

        renderCardHeader: function(form) {
            {% if self.card_header_from_field %}
                {# Get the card header value from a field and escape it #}
                var value = form.fields['{{ self.card_header_from_field|escapejs }}'];
+                if (value===null){


What if the form is not new but the value of the specified field is null? Perhaps we should check form.isNew here instead?

kaedroho · 2017-09-07T08:58:38Z

condensedinlinepanel/edit_handlers.py

@@ -108,6 +109,7 @@ def get_form_extra_data(form):
            'forms': [
                {
                    'id': i,
+                    'as_str': str(form.instance),


Attributes on the form should be camel case and we should also add this to the Form interface in types.ts.

I wonder if as_str is descriptive enough, perhaps we should name it instanceAsStr to make it clearer it's referring to the saved instance rather than the form's current state?

kaedroho · 2017-09-07T09:09:17Z

Thanks both of you for your continued effort on this, it's looking good! Just a couple of small issues remaining.

* some minor js clean up

unexceptable · 2017-09-08T01:01:25Z

@kaedroho I've added six support since I ran into a unicode error with str in py27, and hopefully also addressed all your comments.

Needs a js recompile which i've left for you, since my webpack is different. :P

unexceptable · 2017-09-18T07:03:00Z

@kaedroho Any chance to recompile and review my last few changes? Want to get this merged, since then you can release a new version on pypi and I can start using it in my projects. ;)

kaedroho · 2017-09-29T11:41:49Z

Apologies for the delay (been on holiday). These changes look good to me, thanks!

unexceptable · 2017-09-29T12:03:01Z

@kaedroho more than welcome! And when you publish a new version to pypi I'll start using this in my current project. ;)

kaedroho self-assigned this Aug 15, 2017

kaedroho reviewed Aug 15, 2017

View reviewed changes

unexceptable force-pushed the label branch from 9e34acf to 3cb0cb7 Compare August 16, 2017 04:33

unexceptable force-pushed the label branch from 3cb0cb7 to 61efedc Compare August 16, 2017 05:37

unexceptable mentioned this pull request Aug 16, 2017

Make InlinePanel heading independent of label wagtail/wagtail#3765

Closed

kaedroho reviewed Aug 18, 2017

View reviewed changes

unexceptable force-pushed the label branch from f5b432d to 2f94dc2 Compare August 31, 2017 04:13

Adrian Turjak and others added 3 commits August 31, 2017 16:20

Show label on add button

8c15b28

Recompile JS bundle

25b39b4

unexceptable force-pushed the label branch from 2f94dc2 to 409ff52 Compare August 31, 2017 04:21

unexceptable mentioned this pull request Aug 31, 2017

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

Closed

ababic reviewed Sep 2, 2017

View reviewed changes

ababic pushed a commit to ababic/wagtailmenus that referenced this pull request Sep 3, 2017

reviewing of wagtail-deprecated/wagtail-condensedinlinepanel#16

793b3eb

Better default values for heading, label, new_card_header

0678252

* switch to using verbose_name * add basic translation support for these values * add new_card_header_text arg to panel for an easy user set override.

unexceptable changed the title ~~Show label on add button~~ Label/header customisation and better defaults Sep 4, 2017

ababic reviewed Sep 4, 2017

View reviewed changes

Adrian Turjak added 2 commits September 5, 2017 17:54

use named params for strings

34901be

add TODO note about heading from label

8738217

kaedroho reviewed Sep 7, 2017

View reviewed changes

renamed as_str to instanceAsStr, add six.text_type

b37a03e

* some minor js clean up

Recompile JS

9cd68d4

kaedroho merged commit fc4e03a into wagtail-deprecated:master Sep 29, 2017

Label/header customisation and better defaults #16

Label/header customisation and better defaults #16

Conversation

unexceptable commented Aug 15, 2017 • edited Loading

unexceptable commented Aug 15, 2017

kaedroho commented Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unexceptable commented Aug 15, 2017

unexceptable commented Aug 16, 2017

unexceptable commented Aug 16, 2017

kaedroho Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

kaedroho Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaedroho commented Aug 18, 2017 • edited Loading

unexceptable commented Aug 31, 2017

unexceptable commented Aug 31, 2017

ababic left a comment

Choose a reason for hiding this comment

ababic Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ababic Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unexceptable commented Sep 4, 2017

ababic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ababic Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unexceptable commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaedroho Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

kaedroho commented Sep 7, 2017

unexceptable commented Sep 8, 2017

unexceptable commented Sep 18, 2017

kaedroho commented Sep 29, 2017

unexceptable commented Sep 29, 2017

unexceptable commented Aug 15, 2017 •

edited

Loading

kaedroho commented Aug 15, 2017 •

edited

Loading

kaedroho Aug 18, 2017 •

edited

Loading

kaedroho Aug 18, 2017 •

edited

Loading

kaedroho commented Aug 18, 2017 •

edited

Loading

ababic Sep 2, 2017 •

edited

Loading

ababic Sep 2, 2017 •

edited

Loading

ababic Sep 4, 2017 •

edited

Loading

kaedroho Sep 7, 2017 •

edited

Loading