JSHint
-
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 1) Show all issues
Review Request #10873 — Created Feb. 1, 2020 and updated
Information | |
---|---|
hxqlin | |
Review Board | |
master | |
|
|
0982f18... | |
Reviewers | |
reviewboard | |
This RB creates the new component for improving usability of the admin
page. Currently, it's not very clear what new users need to do when they
come to the admin UI for the first time.This banner displays the various steps that an admin needs to complete
in order to start using their Review Board instance with links to the
corresponding setting and documentation. The banner can be dismissed
using the 'x' in the top right corner and the user can view different
slides by clicking on the corresponding pagination dot or using the
'previous' and 'next' arrows.The pagination dots also show the current state of the instructions
in the slide. Once a slide is viewed, if it's an optional slide, the
corresponding dot is filled in with an info icon. If it's a slide that
requires verification, the dot will be left empty until the step is
completed. Once the step is completed, the dot becomes checked. For the
slide currently being viewed, the dot is solid or filled in.When user navigates to another page after having interacted with the
banner, the state of the banner is preserved. That is, the banner will
show the slide that the user left on and whether other slides were
viewed. The state of the banner is reset if the banner is dismissed.Finally, users can toggle showing or hiding the banner by going to the
Support
setting in the administration page.
Unit test coverage:
- rendering the welcome slide
- rendering a slide with content
- rendering the paginator
- the showSlides method with an index within the slide range, above the
slide range, and below the slide range
- previous, next, pagination dot, and dismiss buttonsManual testing:
- using the prev and next buttons to navigate between slides
- clicking the dismiss button hides the banner
- clicking a pagination dot changes to the correct slide
- links for documentation and to the settings are correct
Description | From | Last Updated |
---|---|---|
I think the RB.Admin.PageView and js-page-model-attrs-items block additions would be great to have in their own review requests. It'd let … |
|
|
Col: 32 Missing semicolon. |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
Col: 11 Missing semicolon. |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
These arguments aren't actually used anywhere. |
|
|
The older functions in this file aren't following modern doc standards, but this should be updated to include any relevant … |
|
|
These should be listed in alphabetical order. |
|
|
Sould be "Return ...", rather than "Get.." This should be written from the point of view of the function. "Get" … |
|
|
This shouldn't be needed. The function can look this up itself through reviewboard.get_manual_url(). |
|
|
These are dicts. |
|
|
When you have a string that needs a single quote, it's best to just use double quotes for the outer … |
|
|
For a lot of our customers, /admin/... won't be a valid URL. That's because Review Board doesn't always live at … |
|
|
No blank line here. |
|
|
This is missing "Returns" and "Args". |
|
|
While we don't always get it right, we generally aim to keep dictionary keys in alphabetical order. This helps with … |
|
|
This needs to return a dictionary, and can't return a RequestContext. You could in Django 1.6, but 1.11 doesn't allow … |
|
|
This should be rb/css/defs.less. |
|
|
Make sure you follow the layout of rules that we use in the other CSS Component files, like page-sidebar.less. There's … |
|
|
We use two space indentation in CSS. |
|
|
We don't want to leak CSS-level presentational rules into the HTML. The idea is that the HTML should be semantic. … |
|
|
This has been deprecated and is no longer used. Instead, you'll want #rb-ns-pages.base.on-shell-mobile-mode. Grep around an updated codebase for examples. |
|
|
Missing a trailig comma here. We always want this on lists of items, so that adding a new line doesn't … |
|
|
Maybe indenting this line will improve the readability? |
![]() |
|
This can end up overriding other onbeforeunload event handlers, which is dangerous. Instead of going this route, we should just … |
|
|
Should be /** |
|
|
Is this line missing a colon at the end? |
![]() |
|
No space on either side of the =. This is the case with argument defaults. |
|
|
ES6 JavaScript should use let or const, rather than var. |
|
|
It's always best to avoid any repeated queries like this. Always fetch the entry you want first and then operate … |
|
|
I mentioned this a bit up above, but on top of the "don't inject CSS rules as class names" bit, … |
|
|
This line could be deleted. |
![]() |
|
These are indented 1 space too far. |
|
|
Some notes about indentation here. We use one space indentation for all HTML. This helps to avoid HTML quickly reaching … |
|
|
Rather than being here, what you'll need to do is put this into the governing PageView class for the page. … |
|
|
This is specific to the admin UI, so it shouldn't be in the base template. Instead, I think you'll want … |
|
|
F401 'reviewboard.get_manual_url' imported but unused |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
F821 undefined name 'manual_url' |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (84 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (85 > 79 characters) |
![]() |
|
E501 line too long (85 > 79 characters) |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
E131 continuation line unaligned for hanging indent |
![]() |
|
F841 local variable 'request' is assigned to but never used |
![]() |
|
F821 undefined name 'usersCount' |
![]() |
|
F821 undefined name 'reviewGroupsCount' |
![]() |
|
F821 undefined name 'defaultReviewersCount' |
![]() |
|
F821 undefined name 'repositoryCount' |
![]() |
|
Col: 2 Missing semicolon. |
![]() |
|
Docstrings use 4 space indentation. |
|
|
This should be bool in Python. |
|
|
This isn't a list of objects, but rather dictionaries. However, we cover what the type is in the Returns block, … |
|
|
No need for the blank line here. |
|
|
No \ needed for the single quote, since we're using double quotes for the string now. |
|
|
Rather than call get_manual_url() over and over, let's call it once at the top and re-use a variable for all … |
|
|
No \ needed for the single quote. |
|
|
The %s/ should just be %s, so we don't end up with a double-slash. |
|
|
No \ needed for the single quote. |
|
|
This can be one statement. |
|
|
This can be one statement. |
|
|
Make sure you're following the pattern we use for other CSS Components. There's a specific documentation and organizational structure we … |
|
|
This should describe what the page is response for by default. |
|
|
Blank line between these. |
|
|
We should avoid using this.options (planning to phase it out), and instead implement an initialize that stores the provided options … |
|
|
No blank line needed here. |
|
|
This should go directly above the declaration of the view, or doc generators won't see it. |
|
|
This should probably live on the view itself. Though, I'd like to see a design that doesn't require this. The … |
|
|
We should be using CSS component classes, not IDs, for any elements in the banner, and referencing those here. |
|
|
Remember to use single quotes instead of double quotes unless the content includes a single quote. Same below. |
|
|
Blank line between these. |
|
|
There's indentation errors here. Remember to keep the {% unindented and to do indentation within the tag. Also, any {% … |
|
|
Blank line after {% load %}. |
|
|
Missing quotes around {{slide.subtitle_url}} |
|
|
Missing quotes around {{slide.doc_url}} |
|
|
As discussed above, we'll want to use CSS classes differently here. We probably don't really need to list these as … |
|
|
There's an exta space before endblock here. |
|
|
There's an extra space before block here. |
|
|
Col: 7 Missing semicolon. |
![]() |
|
Col: 67 Missing semicolon. |
![]() |
|
Col: 39 Missing semicolon. |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
The children of "Args" need to be indented 4 more spaces. |
|
|
Super small thing, but just for consistency, we normally format these as: return json_dumps([ serialize_.... for .... ]) I think, … |
|
|
This will need to document options. We have a special way we do this. Grep around the codebase for Option … |
|
|
Since this will only be needed internally, let's name this _setupBannerAttrs, so it's clear it's not something intended for public … |
|
|
This should document each option using Option Args:. |
|
|
Each one of these calls is expensive, since it'll need to traverse the DOM. We also then re-perform these in … |
|
|
This is where anything involving the DOM or browser should go. That includes cookie setup and DOM lookups/event handling. |
|
|
All comments should be full sentences, with proper punctuation and capitalization. They should also be more explicit. For instance, what … |
|
|
jQuery can simplify this for us. Iteration through elements should be using: $slides.each(index => { ... }); |
|
|
You can combine these: .removeClass('-is-completed -is-empty -is-optional') This will reduce the parsing overhead needed when rebuilding the class list, since … |
|
|
This should be formatted such that all arguments align, like: $.cookie( '...', JSON.stringify(...), { ... }); |
|
|
Let's work rbadmin into the beginning of this, so the cookie is namespaced. Since we repeat the name several times, … |
|
|
We should limit the path to the admin UI, so it's not part of any traffic going to the normal … |
|
|
Doc comment summaries must be limited to one line. Subsequent information can go in following paragraphs. |
|
|
If we're removing the element, we also need to remove the view. You can use this.remove(). |
|
|
Several of these statements can be combined easily. |
|
|
"previous" |
|
|
Space after <%-. |
|
|
Indentation levels look to be incorrect, probably on the <div>. |
|
|
Allowing any code to use cookies during tests (or specifically modifying cookies during tests) can result in unexpected issues when … |
|
|
This should probably be passed directly to the view. Even if not, I don't think other code is referencing banner, … |
|
|
It looks like this is only used within this function, and just when setting up the view, so let's remove … |
|
|
As mentioned above in the implementation, we'll want to store the queried slides directly on the view to avoid repeat … |
|
|
For this, you'll want $slides.eq(0), which will return a jQuery-wrapped object. |
|
|
Ideally we'd use expect(...).toHaveClass('...') That does operate on a DOM element, rather than a jQuery-wrapped element, just FYI. |
|
|
If this is a direct child, we'd want $slide0.children('.rb-c-setup-banner__header') |
|
|
These would be best using toHaveClass. |
|
|
These would be updated to use stored variables on the view. It's okay if those variables are "private" (leading underscore). … |
|
|
This is too long. The function() will need to go on the next line. Same with others below. |
|
|
Multi-line comments must use the following form: /** * ... */ They also must be proper sentences (capitalization, punctuation). Same … |
|
|
These should be operating based on the slides (you can use .filter(...) when working off an existing list of jQuery-wrapped … |
|
|
These tests have a lot of variable definitions for things that are only checked once. It's best to flatten these, … |
|
|
Normally I'd say just combine these statements, but this is another opportunity to use a pre-calculated variable on the view, … |
|
|
These are indented 1 too many spaces. This also needs to call {{block.super}} as the first thing. |
|
|
I might be missing it, but I don't see anything that uses tags from rbadmintags. |
|
|
Col: 76 Missing semicolon. |
![]() |
Add missing template changes and fix lint error
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+200 -1) |
Add manual links and more slides. Implement dismiss button. Add video of banner.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+305 -2) |
||||
Removed Files: |
|||||
Added Files: |
Fix some urls
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+305 -2) |
Some thinking points for reviewers:
- Should authentication settings and inviting team members be combined? From the documentation, they're related.
- Should we even show checkmarks at all? Not all the steps will have them and it might be confusing if we show them on some but not others.
- The documentation says enabling logging is part of the minimum steps for setup so I added a slide for that. However, the docs also said that it's disabled by default but I don't think I enabled it on my local instance and it was already enabled. Not sure if that slide is needed or not.
- Default reviewers is a recommended step in the documentation and seems to be about the same importance as review groups. Should that also be a slide?
- It feels like there's a lot of slides. Do we need to cut down and if so, where?
- Not all the descriptions for the steps take up the same number of lines so the height of the banner and position of the nav buttons moves slightly. Is this an issue and if so, how should we address it? (I'm thinking we'd set the description container to a fixed height?)
Implement pagination dots with caret navigation. Make slide height fixed. Combine slide for review groups with default reviewers. Move dismiss button to top right of slide.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+368 -2) |
Add video of new behaviour.
rb-setup-banner.mov: |
|
---|
Added Files: |
---|
Implement ability to toggle setting to hide/show the banner and add checkmarks for viewed slides in the pagination dots.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+396 -3) |
reviewboard/admin/forms/support_settings.py (Diff revision 6) |
---|
E501 line too long (83 > 79 characters)
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 6) |
---|
Col: 11 Missing semicolon.
Implement banner state persistence.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+432 -3) |
Update description & attach screenshot.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
Refactor template; separated review groups & default reviewers slides.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+427 -3) |
||||
Added Files: |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 8) |
---|
E302 expected 2 blank lines, found 1
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 8) |
---|
E501 line too long (80 > 79 characters)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+428 -3) |
The code looks amazing! Just a few minor suggestions on the comments/docs.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
Maybe indenting this line will improve the readability?
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
Is this line missing a colon at the end?
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
This line could be deleted.
I think this is a great start. There are some coding standards stuff in this review, but there are two big-ticket items to focus on right now:
Your tree is pretty out-of-date, I think, still using some of the old admin UI foundation and I think Django 1.6. It's really important that you keep your branches up-to-date with the latest going on upstream, because this stuff changes every week. As it is, right now, a lot of this code wouldn't be able to merge or even run with the current state of upstream.
I think the overall usability will be enhanced if each step can very clearly indicate whether it has been accomplished, and this necessitates a change to what's returned in the dictionaries. There should be a new key, probably, that contains a boolean result of some query indicating if the thing has been done. Checkmarks should be shown only if the step is completed, not viewed. Otherwise, if I skip a step, I don't know if I've done it. I just know that it shows it was checked.
reviewboard/admin/support.py (Diff revision 9) |
---|
The older functions in this file aren't following modern doc standards, but this should be updated to include any relevant sections, like "Returns".
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
These should be listed in alphabetical order.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
Sould be "Return ...", rather than "Get.."
This should be written from the point of view of the function. "Get" can imply that it's getting something from somewhere else (perhaps another web service), but does not necessarily imply that the caller will be getting anything as a result.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
This shouldn't be needed. The function can look this up itself through
reviewboard.get_manual_url()
.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
When you have a string that needs a single quote, it's best to just use double quotes for the outer string. That's the exception to the "Use single quotes for strings" rule.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
For a lot of our customers,
/admin/...
won't be a valid URL. That's because Review Board doesn't always live at the root of a domain. Sometimes it's athttp://<server>/reviewboard/...=
Plus, in Django 1.11, admin URLs have changed, so these paths may not even work. URLs can and do change sometimes.
Because of these reasons, we don't ever want to hard-code URLs. Instead, it's important that you make use of
reverse()
to get the URL you want. This takes a registered URL name and returns a correct URL.The Django Admin documentation contains a reference on how to specify the URL names you want for a given Django app and model (like
auth
anduser
). For these, it's going to beadmin:auth_user_changelist
and such.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
This is missing "Returns" and "Args".
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
While we don't always get it right, we generally aim to keep dictionary keys in alphabetical order. This helps with adding new keys down the road, since it's very clear where the key should go. Also helps with finding the key you want when reading through the code.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9) |
---|
This needs to return a dictionary, and can't return a
RequestContext
. You could in Django 1.6, but 1.11 doesn't allow it.The change-over from 1.6 to 1.11 happened during this semester, so make sure your codebase is up-to-date and using 1.11. There's a lot of changes that can impact things like custom template tags and template rendering.
reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9) |
---|
This should be
rb/css/defs.less
.
reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9) |
---|
Make sure you follow the layout of rules that we use in the other CSS Component files, like
page-sidebar.less
. There's an organization pattern (and documentation) that we use.
reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9) |
---|
We use two space indentation in CSS.
reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9) |
---|
We don't want to leak CSS-level presentational rules into the HTML. The idea is that the HTML should be semantic. They should use classes that describe what they are or how their behavior should be modified, but not what the resulting style should look like. Instead of something like a
rb-u-display-none
, you might haverb-c-my-component -is-shown
, which conveys the intent but not how it looks.
reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9) |
---|
This has been deprecated and is no longer used. Instead, you'll want
#rb-ns-pages.base.on-shell-mobile-mode
. Grep around an updated codebase for examples.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
Missing a trailig comma here. We always want this on lists of items, so that adding a new line doesn't require editing a prior line.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
This can end up overriding other
onbeforeunload
event handlers, which is dangerous.Instead of going this route, we should just set a cookie when we have new state to set. It doesn't require any server round-trips, so it's fast.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
No space on either side of the
=
. This is the case with argument defaults.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
ES6 JavaScript should use
let
orconst
, rather thanvar
.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
It's always best to avoid any repeated queries like this. Always fetch the entry you want first and then operate on it, when you have multiple things to do.
Also, you can take advantage of chaining in jQuery objects.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9) |
---|
I mentioned this a bit up above, but on top of the "don't inject CSS rules as class names" bit, you also should think of these circle/check-circle parts in a different way. Instead of "What icon do I want," think "what state do I want to represent."
Let the stylesheet handle how it looks. That might be a Font Awesome icon today or something different a year from now, but if we're just toggling classes that represent the state, we can easily change the styling without touching a line of JavaScript.
reviewboard/templates/admin/setup_banner.html (Diff revision 9) |
---|
Some notes about indentation here.
- We use one space indentation for all HTML. This helps to avoid HTML quickly reaching the wrapping points in editors.
- Template tags have their own wraping level. So for instance:
<div ...> <div ...> {% for a in b %} <div ...> {% if a.foo %} ... {% endif %} </div> {% endfor %} </div> </div>
The real language in these files is the Django templating language, so that has its own indentation context (and we place indentation after the
{%
in order to minimize the whitespace going to the browser). The HTML is what's outputted from the template, so it's not relative to the Django indentation level. So it maintains its own indentation context.
reviewboard/templates/admin/setup_banner.html (Diff revision 9) |
---|
Rather than being here, what you'll need to do is put this into the governing
PageView
class for the page.Every page in Review Board is backed by either
RB.PageView
or some subclass. This is defined once in the page, through thejs-page-view-type
template block, along withjs-page-view-options
,js-page-model-type
,js-page-model-attrs
, andjs-page-model-options
. Grep around thereviewboard/templates/
directory for these.So this would go into a base
RB.PageView
subclass for the administration UI. We don't have one specific to the admin UI yet, though, so you will need to create one.This should be
RB.Admin.PageView
, which extendsRB.PageView
, and it should live injs/rb/admin/views/pageView.es6.js
. That would be responsible for constructing aSetupBannerView
in itsrender()
method.All other pages in that directory should be updated to use that instead of
RB.PageView
.
templates/admin/base_site.html
would specify this by default with a{% block js-page-view-type %}RB.Admin.PageView{% endblock %}
.This ensures we have a good setup order, and that the page is fully managed by one class, with minimal injection in the HTML templates.
reviewboard/templates/base.html (Diff revision 9) |
---|
This is specific to the admin UI, so it shouldn't be in the base template.
Instead, I think you'll want to put any markup for this in the
navbar_post
template block.
Fix some issues from review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+469 -3) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 10) |
---|
F401 'reviewboard.get_manual_url' imported but unused
Continue fixing issues from review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+504 -4) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (82 > 79 characters)
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (84 > 79 characters)
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (80 > 79 characters)
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (81 > 79 characters)
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (80 > 79 characters)
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (85 > 79 characters)
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 11) |
---|
E501 line too long (85 > 79 characters)
Fix more issues from review - admin pages currently don't display properly after the addition of
RB.Admin.PageView
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+501 -10) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 12) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 12) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 12) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 12) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 12) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 12) |
---|
E131 continuation line unaligned for hanging indent
Fix rendering issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+496 -11) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 13) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 13) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 13) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 13) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 13) |
---|
E131 continuation line unaligned for hanging indent
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 13) |
---|
E131 continuation line unaligned for hanging indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+496 -11) |
Fix rendering issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+499 -11) |
Provide model attributes to setup banner view
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+541 -16) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 16) |
---|
F841 local variable 'request' is assigned to but never used
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 16) |
---|
F821 undefined name 'reviewGroupsCount'
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 16) |
---|
F821 undefined name 'defaultReviewersCount'
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 16) |
---|
F821 undefined name 'repositoryCount'
Fix flake8 issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+539 -16) |
Updated icons
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 18 (+621 -16) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 18) |
---|
Col: 2 Missing semicolon.
Fix unit tests
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 19 (+633 -19) |
I think the
RB.Admin.PageView
andjs-page-model-attrs-items
block additions would be great to have in their own review requests. It'd let us land those more quickly, and keep this change more focused on the banner.Of course, the
RB.Admin.PageView
itself would be pretty empty in a new change (basically just aRB.Admin.PageView = RB.PageView.extend();
, but we'd be able to keep all those other page view updates isolated to that change, and let other code more quickly build on top of this new foundation so this review request doesn't have to keep up-to-date with external changes.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19) |
---|
This isn't a list of objects, but rather dictionaries.
However, we cover what the type is in the
Returns
block, so we don't really need to say it here.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19) |
---|
No
\
needed for the single quote, since we're using double quotes for the string now.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19) |
---|
Rather than call
get_manual_url()
over and over, let's call it once at the top and re-use a variable for all these.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19) |
---|
The
%s/
should just be%s
, so we don't end up with a double-slash.
reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 19) |
---|
Make sure you're following the pattern we use for other CSS Components. There's a specific documentation and organizational structure we use, that applies to all of these rules.
reviewboard/static/rb/js/admin/views/pageView.es6.js (Diff revision 19) |
---|
This should describe what the page is response for by default.
reviewboard/static/rb/js/admin/views/pageView.es6.js (Diff revision 19) |
---|
We should avoid using
this.options
(planning to phase it out), and instead implement aninitialize
that stores the provided options that are needed from the call.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 19) |
---|
This should go directly above the declaration of the view, or doc generators won't see it.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 19) |
---|
This should probably live on the view itself.
Though, I'd like to see a design that doesn't require this. The JavaScript side should be ignorant. It should just operate off data fed by the Python side (which is why we generate the slide data in the template tag).
Each bit of that slide data generated in Python should take responsibility of indicating if the step is completed, and whether it's required/optional. That would simplify this view quite a bit.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 19) |
---|
We should be using CSS component classes, not IDs, for any elements in the banner, and referencing those here.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 19) |
---|
Remember to use single quotes instead of double quotes unless the content includes a single quote. Same below.
reviewboard/templates/admin/base_site.html (Diff revision 19) |
---|
There's indentation errors here.
Remember to keep the
{%
unindented and to do indentation within the tag.Also, any
{% endblock %}
should contain the name of the block. This helps validate that we've closed the correct block, which is important for more complicated templates.
reviewboard/templates/admin/setup_banner.html (Diff revision 19) |
---|
Missing quotes around
{{slide.subtitle_url}}
reviewboard/templates/admin/setup_banner.html (Diff revision 19) |
---|
Missing quotes around
{{slide.doc_url}}
reviewboard/templates/admin/setup_banner.html (Diff revision 19) |
---|
As discussed above, we'll want to use CSS classes differently here.
We probably don't really need to list these as
__button
so much. Rather, I'd do__prev-button
,__next-button
, and__page-button
. This gives us more useful naming that we can reference with a selector
Add tests and do some code cleanup
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 20 (+971 -19) |
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 20) |
---|
Col: 7 Missing semicolon.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 20) |
---|
Col: 67 Missing semicolon.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 20) |
---|
Col: 39 Missing semicolon.
Fix smaller issues from review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 21 (+968 -19) |
Add depends on links
Depends On: |
|
---|
Start redesign by pushing logic more into Python tags
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 22 (+964 -19) |
Fix how attributes are passed to JS view from Python
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 23 (+969 -19) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 23) |
---|
E501 line too long (83 > 79 characters)
Fix remaining issues from review (CSS classes and using options in
initialize
to get setup banner attrs)
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 24 (+1001 -19) |
Remove overlapping changes submitted in https://reviews.reviewboard.org/r/10978/
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 25 (+973 -10) |
Fix overlapping changes submitted in https://reviews.reviewboard.org/r/10979/
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 26 (+960 -5) |
Make
showSlides
logic less redundant
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 27 (+978 -5) |
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 27) |
---|
The children of "Args" need to be indented 4 more spaces.
reviewboard/admin/templatetags/rbadmintags.py (Diff revision 27) |
---|
Super small thing, but just for consistency, we normally format these as:
return json_dumps([ serialize_.... for .... ])
I think, though, that we shouldn't rely on a serialization function and generator to do this. Instead, let's create a list, loop through, and populate, without the function.
There's a couple reasons for this:
- When a function is defined inline, it will actually be re-created every time the containing function is called. There's overhead to this.
- Generators also add additional overhead. Not much, but they're going to be slower than manually building.
- Function calls have a cost to them as well.
Now this isn't a path that needs to be fast, but re-defining a function on every invocation and then using a generator and function call to generate the slides doesn't buy us anything. It'll be just as clean to replace that
def serialize_setup_banner_slide(slide): ... return serialized_slide
with:
for slide in setup_banner_slides: ... slides.append(serialized_slide)
reviewboard/static/rb/js/admin/views/pageView.es6.js (Diff revision 27) |
---|
This will need to document
options
. We have a special way we do this. Grep around the codebase forOption Args:
.
reviewboard/static/rb/js/admin/views/pageView.es6.js (Diff revision 27) |
---|
Since this will only be needed internally, let's name this
_setupBannerAttrs
, so it's clear it's not something intended for public use.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
This should document each option using
Option Args:
.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
Each one of these calls is expensive, since it'll need to traverse the DOM. We also then re-perform these in other functions like
show()
, which is equally expensive.The better approach is to look for them where we expect them. For instance, the paginator should be a direct child of this view, and each button should be a direct child of the paginator. And then any we want to re-use outside of this function, we should cache on the function itself.
So:
const $paginator = this.$el.children('.rb-c-setup-banner__paginator'); this._$pageButtons = $paginator.children('.rb-c-setup-banner__page-button')
etc.
Further, we should be able to just use
events:
on the view to set up these association, which avoids DOM lookups entirely (we rely on event bubbling instead, which reduces the number of event hooks and traversals).
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
This is where anything involving the DOM or browser should go. That includes cookie setup and DOM lookups/event handling.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
All comments should be full sentences, with proper punctuation and capitalization. They should also be more explicit. For instance, what kind of state are we setting?
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
jQuery can simplify this for us. Iteration through elements should be using:
$slides.each(index => { ... });
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
You can combine these:
.removeClass('-is-completed -is-empty -is-optional')
This will reduce the parsing overhead needed when rebuilding the class list, since it'll only have to parse the current class list and compare once.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
This should be formatted such that all arguments align, like:
$.cookie( '...', JSON.stringify(...), { ... });
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
Let's work
rbadmin
into the beginning of this, so the cookie is namespaced.Since we repeat the name several times, let's make the name a constant on the view.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
We should limit the path to the admin UI, so it's not part of any traffic going to the normal user-facing pages.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
Doc comment summaries must be limited to one line. Subsequent information can go in following paragraphs.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
If we're removing the element, we also need to remove the view. You can use
this.remove()
.
reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27) |
---|
Several of these statements can be combined easily.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
Space after
<%-
.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
Indentation levels look to be incorrect, probably on the
<div>
.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
Allowing any code to use cookies during tests (or specifically modifying cookies during tests) can result in unexpected issues when a developer uses Review Board. So we want to ensure cookies are not set or anything during tests.
So what you should do here is spy on
$.cookie
and$.removeCookie
. We can then check that they were called in our tests, and we can also fake results from the call to influence behavior.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
This should probably be passed directly to the view. Even if not, I don't think other code is referencing
banner
, so we wouldn't want to define a variable for it outside of thisbeforeEach()
.The pattern we typically use when setting up a jQuery-wrapped element from a template is:
$(bannerTemplate({ ... }))
And, if storing as a variable, we'd name it something like
$banner
.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
It looks like this is only used within this function, and just when setting up the view, so let's remove the top-level
let
for this and instead pass it inline during construction.It also needs to use the following code style:
[ { ..., }, { ..., }, ..., ]
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
As mentioned above in the implementation, we'll want to store the queried slides directly on the view to avoid repeat lookups. Because of that, we can remove
$slides
and just reference that variable.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
For this, you'll want
$slides.eq(0)
, which will return a jQuery-wrapped object.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
Ideally we'd use
expect(...).toHaveClass('...')
That does operate on a DOM element, rather than a jQuery-wrapped element, just FYI.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
If this is a direct child, we'd want
$slide0.children('.rb-c-setup-banner__header')
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
These would be best using
toHaveClass
.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
These would be updated to use stored variables on the view.
It's okay if those variables are "private" (leading underscore). Tests are tied to the implementation and can peak.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
This is too long. The
function()
will need to go on the next line.Same with others below.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
Multi-line comments must use the following form:
/** * ... */
They also must be proper sentences (capitalization, punctuation).
Same with others.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
These should be operating based on the slides (you can use
.filter(...)
when working off an existing list of jQuery-wrapped elements), rather than looking for any element matching these.
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
These tests have a lot of variable definitions for things that are only checked once. It's best to flatten these, simplify the code. For example:
expect($paginationDots.index($emptyDot)).toBe(2);
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27) |
---|
Normally I'd say just combine these statements, but this is another opportunity to use a pre-calculated variable on the view, reduce the need for the tests to do its own querying.
reviewboard/templates/admin/base_site.html (Diff revision 27) |
---|
These are indented 1 too many spaces.
This also needs to call
{{block.super}}
as the first thing.
reviewboard/templates/admin/setup_banner.html (Diff revision 27) |
---|
I might be missing it, but I don't see anything that uses tags from
rbadmintags
.
Fix issues from review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 28 (+979 -5) |
reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 28) |
---|
Col: 76 Missing semicolon.
Lint
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 29 (+979 -5) |