• 
      

    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)