Groups: |
|
---|
Implement drag and drop on admin site (back end)
Review Request #5528 — Created Feb. 22, 2014 and submitted
The user can now customize their dashboard by dragging and dropping widgets to move them around.
I opened Review Board on two different browsers, Chrome and Firefox, and logged in as the same user on both browsers. On the Chrome browser, I visited the admin dashboard and changed the ordering of the widgets with drag-and-drop. On the Firefox browser, I also visited the admin dashboard, and I checked to make sure that the positions of the widgets were the same on both browsers, indicating that the positions had been saved on the back end side.
Description | From | Last Updated |
---|---|---|
These lines are too long. They should be no longer than 79 characters. You don't need the list comprehension, though. … |
chipx86 | |
Should keep the trailing comma. |
chipx86 | |
Two blank lines between these. |
chipx86 | |
Doc strings must be in the following format: """One-line summary.""" or: """One-line summary. Multi-line description. """ The summary cannot wrap … |
chipx86 | |
No blank line. |
chipx86 | |
This is basically the same code repeated twice. How about: if widget_type == 'primary': positions_key = 'primary_widget_positions' widgets = primary_widgets … |
chipx86 | |
It's best not to have to fetch this later on every time. I'd suggest doing: positions = profile_data.setdefault(positions_key, {}) Then … |
chipx86 | |
No space after range. Should be: for i in range(len(primary_widgets)): |
chipx86 | |
How does this uniquely represent the widget? Don't we want to operate on the widget_id instead? |
chipx86 | |
No blank line here. The elif is part of the same conditional group. |
chipx86 | |
Should default to None. I'd also make this the very first attribute, and update all the other widgets below to … |
chipx86 | |
Missing a var statement and a semicolon. |
chipx86 | |
Variable names must be lowercase. |
chipx86 | |
Multi-line comments must be in this format: /* * Line * Another line */ |
chipx86 | |
Same comments as above. |
chipx86 | |
This is all basically repeated from above. You should factor that out. |
chipx86 | |
Same as above with the casing. |
chipx86 | |
Same as above for comment style. |
chipx86 | |
One statement per line. |
chipx86 | |
Remove this blank line. |
david | |
These lines have some trailing whitespace, plus the mix of indentation will make some linters unhappy. Let's align it like … |
david | |
While this isn't part of the API, I think we should change this to use POST or PUT for the … |
david | |
You shouldn't need to include any arguments to HttpResponse if you don't need to return any content. |
david | |
This looks like it's from your previous change. You probably need to rebase your branch onto the latest master. |
david | |
Too many spaces between start: and function. |
david | |
This would be slightly more readable as: primary_widget_positions = profile_data.get('primary_widget_positions') if primary_widget_positions: sorted_primary_widgets = sorted( ... |
david | |
Same here. |
david | |
Trailing whitespace. |
david | |
How about adding axis: 'y', to the options for sortable? |
david | |
Neither of the parameters here are used in the function, which can make some linters sad. Can you just remove … |
david | |
How about adding axis: 'y', to the options for sortable? |
david | |
This shouldn't have a comma after it (some browsers, including IE, don't react well to trailing commas in lists of … |
david |
Summary: |
|
||||||
---|---|---|---|---|---|---|---|
Description: |
|
-
-
reviewboard/admin/views.py (Diff revision 1) These lines are too long. They should be no longer than 79 characters.
You don't need the list comprehension, though. You can just use
sorted(...)
. If you need it to be a list, dolist(sorted(...))
. -
-
-
reviewboard/admin/views.py (Diff revision 1) Doc strings must be in the following format:
"""One-line summary."""
or:
"""One-line summary. Multi-line description. """
The summary cannot wrap or exceed the line limit.
-
-
reviewboard/admin/views.py (Diff revision 1) This is basically the same code repeated twice. How about:
if widget_type == 'primary': positions_key = 'primary_widget_positions' widgets = primary_widgets else: # Same for secnodary
# All your other code here. Now it can operate on that data instead of re-calculating it.
-
reviewboard/admin/views.py (Diff revision 1) It's best not to have to fetch this later on every time. I'd suggest doing:
positions = profile_data.setdefault(positions_key, {})
Then you can just do:
positions[widget] = i
-
reviewboard/admin/views.py (Diff revision 1) No space after
range
.Should be:
for i in range(len(primary_widgets)):
-
reviewboard/admin/views.py (Diff revision 1) How does this uniquely represent the widget? Don't we want to operate on the widget_id instead?
-
reviewboard/admin/views.py (Diff revision 1) No blank line here. The
elif
is part of the same conditional group. -
reviewboard/admin/widgets.py (Diff revision 1) Should default to
None
.I'd also make this the very first attribute, and update all the other widgets below to also have this appear first.
-
-
-
reviewboard/static/rb/js/admin.js (Diff revision 1) Multi-line comments must be in this format:
/* * Line * Another line */
-
-
reviewboard/static/rb/js/admin.js (Diff revision 1) This is all basically repeated from above. You should factor that out.
-
-
-
Summary: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+116 -6) |
-
-
-
reviewboard/admin/views.py (Diff revision 5) These lines have some trailing whitespace, plus the mix of indentation will make some linters unhappy. Let's align it like this:
sorted_primary_widgets = sorted( primary_widgets, key=lambda y: ...)
-
reviewboard/admin/views.py (Diff revision 5) While this isn't part of the API, I think we should change this to use POST or PUT for the HTTP request method, instead of GET.
-
reviewboard/admin/views.py (Diff revision 5) You shouldn't need to include any arguments to HttpResponse if you don't need to return any content.
-
reviewboard/static/rb/css/common.less (Diff revision 5) This looks like it's from your previous change. You probably need to rebase your branch onto the latest master.
-
-
This is looking pretty good! Just a couple small suggestions.
-
reviewboard/admin/views.py (Diff revision 7) This would be slightly more readable as:
primary_widget_positions = profile_data.get('primary_widget_positions') if primary_widget_positions: sorted_primary_widgets = sorted( ...
-
-
-
After applying your patch and trying this out, it's pretty awesome. I did notice a few more things, though. Almost there!
-
reviewboard/static/rb/js/admin.js (Diff revision 8) How about adding
axis: 'y',
to the options forsortable
? -
reviewboard/static/rb/js/admin.js (Diff revision 8) Neither of the parameters here are used in the function, which can make some linters sad. Can you just remove the parameter names and make this take no arguments?
-
reviewboard/static/rb/js/admin.js (Diff revision 8) How about adding
axis: 'y',
to the options forsortable
? -
reviewboard/static/rb/js/admin.js (Diff revision 8) This shouldn't have a comma after it (some browsers, including IE, don't react well to trailing commas in lists of things).