Modify djblets.forms.widgets.ListEditWidget to handle any type of widget and form field

Review Request #11858 — Created Oct. 20, 2021 and submitted

Information

Djblets
release-3.x

Reviewers

Previously, ListEditWidget expects values to be strings with a valid
separator, thus it can only be used to edit a list of strings.
The corresponding Django template and Backbone view are hardcoded to
display string values with text inputs.

This change modifies ListEditWidget so that it can modify a list of
any type of values, not just strings. This change includes the creation
of a ListEditField which has the ListEditWidget as its default widget.
This field splits a string into a list of strings, reproducing the
behaviour of the old ListEditWidget. We also introduce a
ListEditDictionaryField, which takes a dictionary and splits it into a
list of (key, value) tuples.

  • Wrote tests for ListEditField and ListEditDictionaryField. Ran all
    tests in djblets.forms.tests with success.
  • Updated tests for ListEditWidget and ListEditView. Ran all Javascript Unit
    tests with success.
  • Performed manual tests.
Summary ID Author
fixed merge conflicts and added updates for
reviewboard5.0
b1e246ac22d4e14e8c5a2e0ae6832f07f8a40c0b Michelle
Description From Last Updated

Typo in summary: dbjlets -> djblets

daviddavid

Please wrap the description/testing done to 70 columns.

daviddavid

Can we change the summary to be "Modify" instead of "Modifying"?

daviddavid

It would be nice to write the testing done as a set of bullet points. Right now it's kind of …

daviddavid

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Will change {{forloop.counter0}} to 0

maubinmaubin

This trailing comma will break older browsers, since it's outputted directly to the browser, and is not in a .es6.js …

chipx86chipx86

Django's MultiWidget also takes in a child widget (well, a list of them). It accepts either a class or an …

chipx86chipx86

So here's a big thing I missed in my prior review (and this is all from the original design, which …

chipx86chipx86

This could be one statement.

chipx86chipx86

No need for parens around name. In fact, (name) is equivalent to name. If we instead had (name,), then it'd …

chipx86chipx86

I suspect we won't want to use attrs itself here. Those are the attributes for the ListEditWidget, which will probably …

chipx86chipx86

We reference self.value_widget many times. Each time we do that, we have to go through the attribute accessor paths in …

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Technically, this makes this a breaking change, which requires a Djblets major version bump (we follow semantic versioning in Djblets, …

chipx86chipx86

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

It's possible that when you are creating ListEditView here, it is not defined, That's why the error Cannot read properties …

akim.ruslanovakim.ruslanov

You're actually storing this as _sep in the __init__ method. We don't need to have any documentation for purely internal/private …

daviddavid

This can be dedented 4 spaces.

daviddavid

Dedent 4 spaces.

daviddavid

This should just be indented 4 spaces, not 7. Even better would be to reformat the docstring so the first …

daviddavid

Because this init method doesn't do anything but call the superclass, we can just get rid of it.

daviddavid

Dedent 4 spaces.

daviddavid

Dedent 4 spaces.

daviddavid

This should have a "Raises" section with django.forms.ValidationError

daviddavid

This new line can go away (plus you added some trailing whitespace).

daviddavid

This new line can go away (plus you added some trailing whitespace).

daviddavid

Please add a module docstring for this.

daviddavid

Please add a module docstring for this.

daviddavid

We generally avoid the python ternary syntax because it's not particularly readable. Let's just make this an if/else.

daviddavid

Add a blank line between these two. For properties, we can also skip "Returns" and just put in: Type: django.forms.widgets.Media …

daviddavid

We do sometimes say "list or unicode" to clarify here.

daviddavid

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

Col: 87 Missing semicolon.

reviewbotreviewbot

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

We don't use var in .es6 files. That said, if you change the .each() to use a fat-arrow function, you …

daviddavid

Col: 62 Missing semicolon.

reviewbotreviewbot

Col: 7 Missing semicolon.

reviewbotreviewbot

Please keep the import list alphabetized

daviddavid

Add a blank line between these two (in python classes, there's one blank line between each thing, and the docstring …

daviddavid

These are each indented one space too many.

daviddavid

Each of these lines needs to be indented one more space.

daviddavid

This new line can be removed.

daviddavid

Add a blank line between these two.

daviddavid

Add a blank line between these two.

daviddavid

f before w

daviddavid

Test docstrings shouldn't have a period at the end, because the test runner will print an ellipsis.

daviddavid

Type definitions don't need an explanation, just the type name. The explanation of the property is in the docstring above …

daviddavid

Add a blank line between these two.

daviddavid

This line can go away.

daviddavid

Instead of passing in len to filter, we should use None. This will do a falsy check instead of actually …

daviddavid

Type here can be jQuery

daviddavid

It looks like this is taking in a jQuery, so we should list that here and call the argument name …

daviddavid

Let's define a variable for $(el) so we don't have to construct the jQuery object twice.

daviddavid

Should this be removed?

daviddavid

We no longer need this on djblets release-3.x or master.

daviddavid

We no longer need this on djblets release-3.x or master.

daviddavid

We need to add some blank lines between all of these.

daviddavid

The outer parens aren't necessary here.

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

maubin
maubin
  1. 
      
  2. Show all issues

    Will change {{forloop.counter0}} to 0

  3. 
      
chipx86
  1. 
      
  2. Show all issues

    This trailing comma will break older browsers, since it's outputted directly to the browser, and is not in a .es6.js file (which would compile suitably for the browsers we need to support).

    1. Given that this is future-looking work that won't ship until RB5.0, I don't think we need to worry about IE11 compat

  3. djblets/forms/widgets.py (Diff revision 2)
     
     
    Show all issues

    Django's MultiWidget also takes in a child widget (well, a list of them). It accepts either a class or an instance in the constructor, and if it's a class, it'll instantiate it.

    I think that's the right pattern, and the right default. Taking in a class just stores a reference to the class in the list of default values for the function.

    Making the default an instance means that:

    1. We instantiate the TextInput and keep this around forever (using memory that may not be needed)
    2. We actually share that default instance across all ListEditWidget instances by default, which could lead to errors with state being clobbered.

    Let's make the default a reference to the class, and then in the constructor we can instantiate it if it's a class or store directly if the caller provided an instance.

  4. djblets/forms/widgets.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    This could be one statement.

  5. djblets/forms/widgets.py (Diff revision 2)
     
     
    Show all issues

    No need for parens around name. In fact, (name) is equivalent to name. If we instead had (name,), then it'd be a tuple with one value (name) in it.

    A tuple is often a correct thing to pass in using %-based formatting, but isn't necessary in this particular case.

    Caveat: If the value being passed in was iterable (a list or tuple, for example), we would need to wrap it as (value,), or the %-formatting would be looking inside of it for values to stuff into the %s and such.

  6. djblets/forms/widgets.py (Diff revision 2)
     
     
    Show all issues

    I suspect we won't want to use attrs itself here. Those are the attributes for the ListEditWidget, which will probably differ.

    A base set of attrs are generally provided upon widget instantiation (or overridden in the widget itself), and then built upon when it's time to render. So starting out, we'd probably want:

    widget_attrs = value_widget.attrs
    

    Which we could pass in to render().

    If we wanted to add our own attributes to each value (say, giving them each a unique ID in the DOM — something worth considering in the future — consider the following an example only), we'd do something like:

    common_widget_attrs = value_widget.attrs
    
    for i, val in enumerate(value_list):
        widget_attrs = {}
    
        if 'id' in attrs:
            widget_attrs['id'] = '%s_%s' % (self.id_for_label(attrs['id']), i)
    
        render(
            ...,
            attrs=value_widget.build_attrs(common_widget_attrs, widget_attrs))
    

    So, taking those base attributes, and then modifying them on each loop so it's suitable for that row.

    For reference:

    • enumerate wraps an iterable, yielding an index and the value for each iteration.
    • id_for_label returns a suitable ID for the widget.
    • build_attrs takes in common attributes and specific attributes and returns a new dictionary combining the two.
    1. Actually, I just looked at the code above a bit, and we are already building attrs as the per-child-widget attrs. So let's keep doing that. (I'm not sure that's strictly correct, but it's current behavior). So much of the above can be ignored.

  7. djblets/forms/widgets.py (Diff revision 2)
     
     
    Show all issues

    We reference self.value_widget many times. Each time we do that, we have to go through the attribute accessor paths in the class. Not particularly slow, but not worth doing. Let's pull out value_widget into a local variable before the loop above, so accesses are fast.

  8. djblets/forms/widgets.py (Diff revision 2)
     
     
    Show all issues

    Missing a trailing comma.

  9. Show all issues

    Technically, this makes this a breaking change, which requires a Djblets major version bump (we follow semantic versioning in Djblets, at least nowadays).

    This is probably fine, since we're targeting Djblets 3.0 for this work, but we need to document this with the following before the Args section:

    Version Changed:
        3.0:
        * Removed the `inputAttrs` option.
        * Added the `renderedDefaultRow` option.
    

    (Also, renderedDefaultRow will need to be documented.)

  10. 
      
chipx86
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 2)
     
     
     
     
    Show all issues

    So here's a big thing I missed in my prior review (and this is all from the original design, which makes modernizing this tricky).

    This code is still assuming that we're dealing strictly with individual strings (and that we can just split them). That isn't safe, as the widget may be returning an integer, a list, a dictionary, or even string-encoded content that may get split wrong.

    However, we don't want to break existing consumers.

    The problem comes down to the original design. This assumed it was only ever going to work with strings, and that the data being fed in was a string that had a valid separator.

    It should have been designed to work in conjunction with a matching field that prepared the value before it got to render(), turning whatever the content would be into a list of values that pertained to the widgets.

    Basically, it should be the job of the field to prepare data and the job of a widget to render it. The widget shouldn't have to be parsing anything.

    So I think we can work our way toward this design by doing the following:

    1. In render(), check if value is a list. If so, we're good. This is modern. If not, convert it to a list through this existing logic we have here. (Update the code to reuse value, instead of value_list, to make it clear that this is what we expect of it.)
    2. Add a comment saying that we plan to deprecate this behavior. In a future release, we'll enable deprecation warnings for this code path.
    3. Introduce a djblets.forms.fields.ListEditField. This will be the new field intended to work with ListEditWidget. Its job will be to take in a string and convert it to values, and to associate ListEditWidget as the default widget. Subclasses (such as your field for the Pygments work) can do that conversion however is needed. By default, it should split in the same way that the widget otherwise would.

    This effectively gives us a migration path to that ideal world in which the widget just renders, and the field handles the data processing. It would then be really easy to subclass these and get the data and widget setup in the right form.

    That may sound like a lot, but the field work isn't going to be too bad. Not documented as well as it should be, but I can guide you through any of it.

  3. 
      
maubin
Review request changed
Change Summary:

Create ListEditField and ListEditDictionaryField and their tests

Description:
~  

Previously, ListEditWidget expects values to be strings rendered with a TextInput widget.

~   Initial modifications to list edit widget so that values can be rendered by any widget

  ~

Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.

  ~
  +

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change also includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

  +
  +

TODO:

  + - ModifyListEditWidget and its template and backbone view (and tests for each of those)

Testing Done:
~  

Testing on dev server, "Add Item" button saves the entire form instead of just adding another entry to the list widget (not sure why)

  ~

Added tests for ListEditField and ListEditDictionaryField, ran successfully

Commits:
Summary ID Author
initial modifications to list edit widget
7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle
modified html classes and names
7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
initial modifications to list edit widget
7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle
modified html classes and names
7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
create listeditfield
8c5a5e698f733d5e974367f9c85d2f436353873c Michelle
create listeditdictionaryfield
8d377e93fc24ccd29beac106c037d4facd825b2e Michelle
add tests for listEditField
2df3771437170564840a4c768e49bedca3881606 Michelle
add listEditDictionary tests and some typo fixes
6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
maubin
maubin
akim.ruslanov
  1. 
      
  2. djblets/forms/templates/djblets_forms/list_edit_widget.html (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    It's possible that when you are creating ListEditView here, it is not defined, That's why the error Cannot read properties of undefined (reading 'ListEditView') happens? I would try to see if I can do something like console.log(Djblets.Forms) just to see if the class is properly imported/accessible within this HTML. You might need to add extra code to specify where to get the ListEditView from

  3. 
      
mxwang
  1. 
      
  2. Are you calling/importing the Form correctly? Adding the correct classes (ex. multipart/form) when rendering it in the HTML? Does new items in the listeditview require a different form of parsing than just parsing the text?

    I would also console.log() it out to see if the result is what you're expecting, and check the types of the results too.

  3. 
      
maubin
Review request changed
Change Summary:

Done all the logic for ListEditWidget, need to modify tests

Description:
   

Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.

   
~  

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change also includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

  ~

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

   
   

TODO:

~   - ModifyListEditWidget and its template and backbone view (and tests for each of those)

  ~ - Modify tests for ListEditWidget, its template and backbone view

Commits:
Summary ID Author
initial modifications to list edit widget
7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle
modified html classes and names
7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
create listeditfield
8c5a5e698f733d5e974367f9c85d2f436353873c Michelle
create listeditdictionaryfield
8d377e93fc24ccd29beac106c037d4facd825b2e Michelle
add tests for listEditField
2df3771437170564840a4c768e49bedca3881606 Michelle
add listEditDictionary tests and some typo fixes
6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle
fixed flake8 issues
aa46cef736df948002f411c0457144bc224102c5 Michelle
add get_context and value_from_datadict methods
530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle
working on javascript logic for dynamically adding and removing values in listeditwidget
c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle
initial modifications to list edit widget
7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle
modified html classes and names
7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
create listeditfield
8c5a5e698f733d5e974367f9c85d2f436353873c Michelle
create listeditdictionaryfield
8d377e93fc24ccd29beac106c037d4facd825b2e Michelle
add tests for listEditField
2df3771437170564840a4c768e49bedca3881606 Michelle
add listEditDictionary tests and some typo fixes
6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle
fixed flake8 issues
aa46cef736df948002f411c0457144bc224102c5 Michelle
add get_context and value_from_datadict methods
530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle
working on javascript logic for dynamically adding and removing values in listeditwidget
c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle
kinda working but need to fix getting proper index for data-list-index and names
fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle
done logic for adding and removing values in listeditwidget
715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle
address issues from review request
bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
  1. I know this is still marked WIP, but it's looking really solid so I did a pretty thorough review.

  2. Show all issues

    Typo in summary: dbjlets -> djblets

  3. Show all issues

    Please wrap the description/testing done to 70 columns.

  4. djblets/forms/fields.py (Diff revision 7)
     
     
     
    Show all issues

    You're actually storing this as _sep in the __init__ method. We don't need to have any documentation for purely internal/private attributes, unless they're particularly confusing or complicated.

  5. djblets/forms/fields.py (Diff revision 7)
     
     
    Show all issues

    This can be dedented 4 spaces.

  6. djblets/forms/fields.py (Diff revision 7)
     
     
    Show all issues

    Dedent 4 spaces.

  7. djblets/forms/fields.py (Diff revision 7)
     
     
    Show all issues

    This should just be indented 4 spaces, not 7. Even better would be to reformat the docstring so the first line fits on one line and move any details into a second paragraph.

  8. djblets/forms/fields.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Because this init method doesn't do anything but call the superclass, we can just get rid of it.

  9. djblets/forms/fields.py (Diff revision 7)
     
     
    Show all issues

    Dedent 4 spaces.

  10. djblets/forms/fields.py (Diff revision 7)
     
     
    Show all issues

    Dedent 4 spaces.

  11. djblets/forms/fields.py (Diff revision 7)
     
     
    Show all issues

    This should have a "Raises" section with django.forms.ValidationError

  12. Show all issues

    This new line can go away (plus you added some trailing whitespace).

  13. Show all issues

    This new line can go away (plus you added some trailing whitespace).

  14. Show all issues

    Please add a module docstring for this.

    1. What should the docstring say? I looked at the other tests in djblets.forms.tests but didn't see any files that had a module docstring

    2. It can probably be the same as the class docstring.

  15. Show all issues

    Please add a module docstring for this.

  16. djblets/forms/widgets.py (Diff revision 7)
     
     
     
    Show all issues

    We generally avoid the python ternary syntax because it's not particularly readable. Let's just make this an if/else.

  17. djblets/forms/widgets.py (Diff revision 7)
     
     
     
    Show all issues

    Add a blank line between these two.

    For properties, we can also skip "Returns" and just put in:

    Type:
        django.forms.widgets.Media
    

    because callers can use it as if it were just a plain attribute and not a method.

    1. there's another media() method in this file for the ConditionsWidget that has "Returns". Should we change this too?

    2. Sure.

  18. djblets/forms/widgets.py (Diff revision 7)
     
     
    Show all issues

    We do sometimes say "list or unicode" to clarify here.

  19. Show all issues

    Trailing whitespace on this line.

  20. Show all issues

    Trailing whitespace on this line.

  21. Show all issues

    Trailing whitespace on this line.

  22. Show all issues

    Trailing whitespace on this line.

  23. Show all issues

    Trailing whitespace on this line.

  24. Show all issues

    Trailing whitespace on this line.

  25. Show all issues

    We don't use var in .es6 files. That said, if you change the .each() to use a fat-arrow function, you can just use this directly instead of aliasing it to self.

  26. 
      
maubin
maubin
maubin
Review request changed
Summary:
[WIP] Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form field
Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form field
Description:
   

Previously, ListEditWidget expects values to be strings with a valid

    separator, thus it can only be used to edit a list of strings.
    The corresponding Django template and Backbone view are hardcoded to
    display string values with text inputs.

   
   

This change modifies ListEditWidget so that it can modify a list of

    any type of values, not just strings. This change includes the creation
    of a ListEditField which has the ListEditWidget as its default widget.
    This field splits a string into a list of strings, reproducing the
    behaviour of the old ListEditWidget. We also introduce a
    ListEditDictionaryField, which takes a dictionary and splits it into a
    list of (key, value) tuples.

-  
-  

TODO:

-   - Modify tests for ListEditWidget, its template and backbone view

Testing Done:
~  

Added tests for ListEditField and ListEditDictionaryField.

  ~

Added tests for ListEditField and ListEditDictionaryField. Ran all

  + tests in djblets.forms.tests with success.
  + Updated tests for ListEditWidget and ListEditView. Ran all Javasccript
  + Unit tests with success (2 fail but they also fail on the master
  + branch)

Commits:
Summary ID Author
initial modifications to list edit widget
7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle
modified html classes and names
7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
create listeditfield
8c5a5e698f733d5e974367f9c85d2f436353873c Michelle
create listeditdictionaryfield
8d377e93fc24ccd29beac106c037d4facd825b2e Michelle
add tests for listEditField
2df3771437170564840a4c768e49bedca3881606 Michelle
add listEditDictionary tests and some typo fixes
6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle
fixed flake8 issues
aa46cef736df948002f411c0457144bc224102c5 Michelle
add get_context and value_from_datadict methods
530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle
working on javascript logic for dynamically adding and removing values in listeditwidget
c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle
kinda working but need to fix getting proper index for data-list-index and names
fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle
done logic for adding and removing values in listeditwidget
715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle
address issues from review request
bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle
handle old behavior of rejoining values into a string
4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle
applied review request feedback
47621a8995c54c783c375169d32fd0d20fe12b8d Michelle
initial modifications to list edit widget
7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle
modified html classes and names
7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
create listeditfield
8c5a5e698f733d5e974367f9c85d2f436353873c Michelle
create listeditdictionaryfield
8d377e93fc24ccd29beac106c037d4facd825b2e Michelle
add tests for listEditField
2df3771437170564840a4c768e49bedca3881606 Michelle
add listEditDictionary tests and some typo fixes
6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle
fixed flake8 issues
aa46cef736df948002f411c0457144bc224102c5 Michelle
add get_context and value_from_datadict methods
530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle
working on javascript logic for dynamically adding and removing values in listeditwidget
c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle
kinda working but need to fix getting proper index for data-list-index and names
fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle
done logic for adding and removing values in listeditwidget
715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle
address issues from review request
bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle
handle old behavior of rejoining values into a string
4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle
applied review request feedback
cb83b214f746bcff67214a847e213091490b03b1 Michelle
done testing and small improvements
f95bcbc519ddd5ec851e88ce2e69989a3028c0dc Michelle

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

maubin
david
  1. 
      
  2. djblets/forms/fields.py (Diff revision 10)
     
     
    Show all issues

    Please keep the import list alphabetized

  3. djblets/forms/fields.py (Diff revision 10)
     
     
     
    Show all issues

    Add a blank line between these two (in python classes, there's one blank line between each thing, and the docstring counts as a thing).

  4. Show all issues

    These are each indented one space too many.

  5. Show all issues

    Each of these lines needs to be indented one more space.

  6. Show all issues

    This new line can be removed.

  7. Show all issues

    Add a blank line between these two.

  8. djblets/forms/tests/test_list_edit_field.py (Diff revision 10)
     
     
     
    Show all issues

    Add a blank line between these two.

  9. djblets/forms/tests/test_list_edit_widget.py (Diff revision 10)
     
     
     
    Show all issues

    f before w

  10. Show all issues

    Test docstrings shouldn't have a period at the end, because the test runner will print an ellipsis.

  11. djblets/forms/widgets.py (Diff revision 10)
     
     
    Show all issues

    Type definitions don't need an explanation, just the type name. The explanation of the property is in the docstring above this.

  12. djblets/forms/widgets.py (Diff revision 10)
     
     
     
    Show all issues

    Add a blank line between these two.

  13. djblets/forms/widgets.py (Diff revision 10)
     
     
    Show all issues

    This line can go away.

  14. djblets/forms/widgets.py (Diff revision 10)
     
     
    Show all issues

    Instead of passing in len to filter, we should use None. This will do a falsy check instead of actually checking the length of the string, which is ever so slightly faster.

  15. Show all issues

    Type here can be jQuery

  16. Show all issues

    It looks like this is taking in a jQuery, so we should list that here and call the argument name $entry

  17. Show all issues

    Let's define a variable for $(el) so we don't have to construct the jQuery object twice.

  18. Show all issues

    Should this be removed?

  19. 
      
maubin
maubin
david
  1. 
      
  2. Show all issues

    Can we change the summary to be "Modify" instead of "Modifying"?

  3. Show all issues

    It would be nice to write the testing done as a set of bullet points. Right now it's kind of hard to read.

  4. Show all issues

    We no longer need this on djblets release-3.x or master.

  5. Show all issues

    We no longer need this on djblets release-3.x or master.

  6. djblets/forms/widgets.py (Diff revision 12)
     
     
     
     
     
     
     
     
    Show all issues

    We need to add some blank lines between all of these.

  7. 
      
maubin
david
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 13)
     
     
    Show all issues

    The outer parens aren't necessary here.

  3. 
      
maubin
david
  1. Ship It!
  2. 
      
maubin
maubin
david
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (6b4674c)