Add the ListEditWidget

Review Request #8982 — Created June 1, 2017 and submitted

Information

Djblets
release-0.10.x

Reviewers

The ListEditWidget provides an interface for editing a delimited
list of values as a list of input fields.

Used this with an upcoming change for Review Board.


Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Col: 73 Missing semicolon.

reviewbotreviewbot

Col: 72 Expected ')' and instead saw ';'.

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

No space between function and ()

daviddavid

Shouldn't have trailing commas in non-ES6 code.

daviddavid

, optional

daviddavid

, optional

daviddavid

You're using the variable name item in both the inner and outer loops. How about different names?

daviddavid

Trailing comma?

daviddavid

Missing Args/Returns

daviddavid

Trailing whitespace.

daviddavid

No spaces around the =

daviddavid

We have a default value (empty object) for options. What happens if this is constructed with that default? It looks …

daviddavid

Trailing comma?

daviddavid

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Col: 68 Missing semicolon.

reviewbotreviewbot

Col: 64 Missing semicolon.

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Can we change this around a little bit so we have: const $entry = $(...) .insertBefore(this._$addBtn); $entry .find(...) .on(...) .end() …

daviddavid
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. Screenshots?

  2. Show all issues

    No space between function and ()

  3. Show all issues

    Shouldn't have trailing commas in non-ES6 code.

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

    , optional

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

    , optional

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

    You're using the variable name item in both the inner and outer loops. How about different names?

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

    Trailing comma?

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

    Missing Args/Returns

  9. Show all issues

    Trailing whitespace.

  10. Show all issues

    No spaces around the =

  11. djblets/static/djblets/js/forms/views/listEditView.es6.js (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    We have a default value (empty object) for options. What happens if this is constructed with that default? It looks like we probably would end up with broken undefineds in the DOM.

    1. This view is not intended to be used outside of the widget.

    2. In that case, options probably shouldn't have a default?

  12. Show all issues

    Trailing comma?

  13. 
      
brennie
Review request changed

Change Summary:

Added unit tests. Fixed some bugs. Addressed David's feedback.

Diff:

Revision 5 (+469 -1)

Show changes

Added Files:

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. djblets/static/djblets/js/forms/views/listEditView.es6.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can we change this around a little bit so we have:

    const $entry = $(...)
        .insertBefore(this._$addBtn);
    
    $entry
        .find(...)
            .on(...)
        .end()
        .find(...)
            .on(...)
        .end();
    
  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (f10b5ac)
Loading...