Add smarter objects selector widget into Djblets

Review Request #10214 — Created Oct. 10, 2018 and submitted

Information

Djblets
master
25eac0d...

Reviewers

This change adds a smarter objects selector widget from Review Board 4.0
into Djblets. This is a form field widget and Backbone view, which must
be extended to be useful.

See /r/10215.

I ran all tests with ./tests/runtests.py. No errors were thrown.

With the /r/10215 Review Board patch, I navigated to
admin/db/scmtools/repository/add/. Next to "Users with access:", the
smarter user selector widget appeared. This widget extends the smarter
objects selector widget.

Description From Last Updated

Remove period from Summary.

gojeffchogojeffcho

Blank line between these.

brenniebrennie

You need to move this template into Djblets as well. If anyone were to use this outside of Review Board, …

brenniebrennie

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

This needs to be Djblets. not RB..

brenniebrennie

"This is to be used" sounds a bit awkward - if it must be used, I would specify that, or …

gojeffchogojeffcho

This comment is really confusing to me, especially the part that says "but the siteconfig field template special cases when …

shovenshoven

.related-object-selector might be a more clear class name (since it's for the relatedObjectSelectorView)?

shovenshoven

Could also use _.each($items, function($item) {...}); or $items.forEach(function($item) {...}); rather than a for loop (I typically subscribe to airbnb's Javascript …

shovenshoven

Typo: initalize --> initialize

ilawilaw

Capitalization is off here. "size" should be lower case, and "Related" should be upper.

daviddavid

Col: 38 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 25 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 29 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 28 Expected '{' and instead saw 'score'.

reviewbotreviewbot

Col: 37 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 36 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 32 'a_value' is defined but never used.

reviewbotreviewbot

Col: 41 'b_value' is defined but never used.

reviewbotreviewbot

Col: 33 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 40 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 56 Expected '{' and instead saw 'options'.

reviewbotreviewbot

Col: 52 Expected '{' and instead saw 'options'.

reviewbotreviewbot

Col: 64 Expected '{' and instead saw 'options'.

reviewbotreviewbot

Col: 26 'value' is defined but never used.

reviewbotreviewbot

Col: 48 'calculateScore' is defined but never used.

reviewbotreviewbot

Col: 22 Expected '{' and instead saw 'search'.

reviewbotreviewbot

Col: 9 'cmp' was used before it was defined.

reviewbotreviewbot

Col: 20 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 20 Expected '{' and instead saw 'return'.

reviewbotreviewbot

Col: 9 'extend' was used before it was defined.

reviewbotreviewbot

Col: 26 Expected '{' and instead saw 'continue'.

reviewbotreviewbot

Col: 9 'trim' was used before it was defined.

reviewbotreviewbot

Col: 9 'escape_regex' was used before it was defined.

reviewbotreviewbot

Col: 9 'is_array' was used before it was defined.

reviewbotreviewbot

Col: 9 'DIACRITICS' was used before it was defined.

reviewbotreviewbot

Col: 9 'asciifold' was used before it was defined.

reviewbotreviewbot

Col: 9 'utils' was used before it was defined.

reviewbotreviewbot

Col: 25 'endbit' is defined but never used.

reviewbotreviewbot

Col: 13 'key' is defined but never used.

reviewbotreviewbot

Col: 17 'timeout_blur' is defined but never used.

reviewbotreviewbot
brennie
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 1)
     
     
     

    Blank line between these.

  3. djblets/forms/widgets.py (Diff revision 1)
     
     

    This is used inside of Django to extract the value from the data and files dictionaries based on the name of the field (name).

    See the docs.

  4. 
      
brennie
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 1)
     
     

    You need to move this template into Djblets as well. If anyone were to use this outside of Review Board, they would get a template not found error.

  3. 
      
skaefer143
Review request changed

Change Summary:

I moved the JS Widgets into Djblets, and added a new initialization argument called using_avatars. This is because we can't assume in Djblets that the widget is using an avatar service.
I think this works for moving templates and JS files into Djblets, but let me know if I'm wrong.

Commit:

-a14bf9f21bfea84a84f52667c1081f2c3b6afead
+250c8898785a7c90a6067e09db05573a4873e8a1

Diff:

Revision 2 (+560)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

skaefer143
skaefer143
skaefer143
skaefer143
brennie
  1. 
      
  2. This needs to be Djblets. not RB..

  3. 
      
skaefer143
skaefer143
gojeffcho
  1. 
      
  2. Remove period from Summary.

  3. djblets/forms/widgets.py (Diff revision 7)
     
     

    "This is to be used" sounds a bit awkward - if it must be used, I would specify that, or if it is recommended but permissible, should be used, maybe? Or is it potentially better to change it to the imperative voice and say "Use this with..."?

  4. 
      
shoven
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 7)
     
     

    This comment is really confusing to me, especially the part that says "but the siteconfig field template special cases when is_hidden is True" (it seems like it's missing a verb perhaps?). If you could reword this comment a bit to make it a bit more clear that would be great!

  3. .related-object-selector might be a more clear class name (since it's for the relatedObjectSelectorView)?

  4. Could also use _.each($items, function($item) {...}); or $items.forEach(function($item) {...}); rather than a for loop (I typically subscribe to airbnb's Javascript guidelines which recommends something like this over for loops but maybe a mentor can weigh in on their preference for Javascript style).

    1. I didn't actually write this code, it came over from a previous implementation. I think this code has to do with placing items in the list if the text string is a different length in a different locale. I tried implementing your code, but it didn't work for me. I was getting some weird glitches, with the widget unable to select and display multiple objects. I might just be doing something wrong also. I'm going to leave the code as is. I did learn something new today though!

  5. 
      
skaefer143
david
  1. 
      
  2. djblets/static/djblets/css/defs.less (Diff revision 8)
     
     

    Capitalization is off here. "size" should be lower case, and "Related" should be upper.

  3. 
      
ilaw
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 8)
     
     

    Typo: initalize --> initialize

  3. Maybe change wording to "to make necessary API requests"

  4. 
      
skaefer143
skaefer143
skaefer143
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
  1. Going to make some small compat updates and land this. Thanks!

  2. 
      
skaefer143
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (17b1668)
Loading...