- 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:
-
Implement drag and drop on admin site (back end){WIP] Implement drag and drop on admin site (back end)
- Description:
-
~ [WIP] The user can now customize their dashboard by dragging and dropping widgets to move them around.
~ The user can now customize their dashboard by dragging and dropping widgets to move them around.
-
-
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(...))
. -
-
-
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.
-
-
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.
-
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
-
-
-
-
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.
-
-
-
-
-
-
-
-
- Summary:
-
[WIP] Implement drag and drop on admin site (back end)Implement drag and drop on admin site (back end)
- Diff:
-
Revision 3 (+125 -8)
-
-
-
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: ...)
-
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.
-
You shouldn't need to include any arguments to HttpResponse if you don't need to return any content.
-
This looks like it's from your previous change. You probably need to rebase your branch onto the latest master.
-
-
After applying your patch and trying this out, it's pretty awesome. I did notice a few more things, though. Almost there!
-
-
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?
-
-
This shouldn't have a comma after it (some browsers, including IE, don't react well to trailing commas in lists of things).