Add a support status banner to the admin dashboard
Review Request #8064 — Created March 17, 2016 and submitted
| Information | |
|---|---|
| brennie | |
| Review Board | |
| release-2.5.x | |
|
|
|
| Reviewers | |
| reviewboard | |
The support banner shows the current support status, via
beanbaginc.com's API. While the data is being retrieved, a spinner is
being shown.
Tested the banner with:
- an active support contract;
- an inactive support contract; and
- no support contract.
| Description | From | Last Updated |
|---|---|---|
|
Style-wise, it'd be nice to actually have this appear like a draft banner. Something flush against the left, top, and … |
 chipx86
|
|
|
Undo this change. |
 david
|
|
|
Add another blank line here. |
david
|
|
|
Alignment is a little funny here. Since there's only one item in the object, can you put that all on … |
david
|
|
|
Doc comment? |
david
|
|
|
Doc comments throughout? |
david
|
|
|
Can you use $.param to format the querystring args? |
david
|
|
|
Can you assert that instance is null? |
david
|
|
|
You can flip this around and default options into your new object, which makes it so you can skip the … |
david
|
|
|
Since this is global, instead of assigning to var, we probably should be doing window.SUPPORT_DATA = .... |
david
|
|
|
Alphabetical order. |
chipx86
|
|
|
Maybe I'm thinking a different language, but I thought you had to wrap new Blah in parens if you wanted … |
chipx86
|
|
|
Rather than doing this and building the script yourself, you can just do: $.ajax({ url: RB.SupportBannerView.supportURL, data: { 'support-data': SUPPORT_DATA, … |
chipx86
|
|
|
This isn't a boolean. If we want to check it against an empty string, we should do that explicitly. |
chipx86
|
|
|
You can just do var SUPPORT_DATA, rather than explicitly attaching it to window. |
chipx86
|
|
|
Typo. = |
chipx86
|
|
|
Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4? |
 imadueme
|
|
|
Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4? |
imadueme
|
|
|
Doesn't look like you added this, but, are vim preferences supposed to be in here? |
imadueme
|
|
|
Blank line in between these. |
david
|
-
Can you move the admin.js refactor into a separate change?
It also looks like
backbone-1.0.0.min.jsis no longer a minified file. What happened there? -
-
-
reviewboard/static/rb/js/admin/admin.js (Diff revision 1) Alignment is a little funny here. Since there's only one item in the object, can you put that all on one line?
-
-
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1) Doc comments throughout?
-
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1) Can you use $.param to format the querystring args?
-
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1) Can you assert that
instanceisnull? -
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 1) You can flip this around and default
optionsinto your new object, which makes it so you can skip the|| {}part. -
reviewboard/templates/admin/dashboard.html (Diff revision 1) Since this is global, instead of assigning to var, we probably should be doing
window.SUPPORT_DATA = ....
Description: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Depends On: |
|
||||||||||||||||||
Diff: |
Revision 2 (+105 -3) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/supportBannerView.js Tool: Pyflakes Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/supportBannerView.js
-
Tool: Pyflakes Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/webhookFormView.js reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/js/admin/repositoryform.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/webhookFormView.js reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/js/admin/repositoryform.js
-
Tool: Pyflakes Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js
-
-
-
reviewboard/static/rb/js/admin/admin.js (Diff revision 4) Maybe I'm thinking a different language, but I thought you had to wrap
new Blahin parens if you wanted to immediately call a function on it?I think I'd prefer the standard thing of two statements: One instantiating, one rendering. Easier to debug if things go wrong, too.
-
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 4) Rather than doing this and building the script yourself, you can just do:
$.ajax({ url: RB.SupportBannerView.supportURL, data: { 'support-data': SUPPORT_DATA, }, dataType: 'jsonp', jsonpCallback: 'RB.SupportBannerView.instance.receive' });That will handle all the work of doing the JSONP stuff, along with handling cache busting.
-
reviewboard/static/rb/js/admin/views/supportBannerView.js (Diff revision 4) This isn't a boolean. If we want to check it against an empty string, we should do that explicitly.
-
reviewboard/templates/admin/dashboard.html (Diff revision 4) You can just do
var SUPPORT_DATA, rather than explicitly attaching it towindow. -
Diff: |
Revision 5 (+202 -8)
|
||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Removed Files: |
|||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/css/ui/boxes.less reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/css/defs.less Tool: PEP8 Style Checker Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/css/ui/boxes.less reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/css/defs.less
-
-
reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 5) Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?
-
reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 5) Is the alignment a bit off here? Shouldn't it be 2 spaces in, not 4?
-
reviewboard/static/rb/css/pages/admin-dashboard.less (Diff revision 5) Doesn't look like you added this, but, are vim preferences supposed to be in here?
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/ui/boxes.less Tool: Pyflakes Ignored Files: reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/ui/boxes.less
-
Tool: Pyflakes Ignored Files: reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/ui/boxes.less Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/css/defs.less reviewboard/static/rb/css/ui/boxes.less
Tool: Pyflakes Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/webhookFormView.js reviewboard/static/rb/js/admin/models/supportContractModel.js reviewboard/static/lib/js/backbone-1.0.0.min.js reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/js/admin/repositoryform.js Tool: PEP8 Style Checker Processed Files: reviewboard/admin/views.py reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/admin/admin.js reviewboard/static/rb/css/pages/admin-dashboard.less reviewboard/static/rb/js/admin/views/webhookFormView.js reviewboard/static/rb/js/admin/models/supportContractModel.js reviewboard/static/lib/js/backbone-1.0.0.min.js reviewboard/templates/admin/dashboard.html reviewboard/static/rb/js/admin/views/supportBannerView.js reviewboard/static/rb/js/admin/repositoryform.js