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?)

The code looks amazing! Just a few minor suggestions on the comments/docs.

Maybe indenting this line will improve the readability?

Is this line missing a colon at the end?

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.

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 sections, like "Returns".

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" 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.

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 string. That's the exception to the "Use single quotes for strings" rule.

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 at http://<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 and user). For these, it's going to be admin:auth_user_changelist and such.

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 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.

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.

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 an organization pattern (and documentation) that we use.

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. 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 have rb-c-my-component -is-shown, which conveys the intent but not how it looks.

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 require editing a prior line.

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.

Should be /**

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 on it, when you have multiple things to do.
Also, you can take advantage of chaining in jQuery objects.

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.

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 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.

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 the js-page-view-type template block, along with js-page-view-options, js-page-model-type, js-page-model-attrs, and js-page-model-options. Grep around the reviewboard/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 extends RB.PageView, and it should live in js/rb/admin/views/pageView.es6.js. That would be responsible for constructing a SetupBannerView in its render() 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.

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.

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 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 a RB.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.

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, so we don't really need to say it here.

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 these.

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 use, that applies to all of these rules.

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 that are needed from the call.

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 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.

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 {% 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.

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 __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

There's an exta space before endblock here.

There's an extra space before block here.