Admin UI blank state banner

Review Request #10873 — Created Feb. 1, 2020 and updated

Information

Review Board
master
0982f18...

Reviewers

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 buttons

Manual 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 …

chipx86chipx86

Col: 32 Missing semicolon.

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

These arguments aren't actually used anywhere.

chipx86chipx86

The older functions in this file aren't following modern doc standards, but this should be updated to include any relevant …

chipx86chipx86

These should be listed in alphabetical order.

chipx86chipx86

Sould be "Return ...", rather than "Get.." This should be written from the point of view of the function. "Get" …

chipx86chipx86

This shouldn't be needed. The function can look this up itself through reviewboard.get_manual_url().

chipx86chipx86

These are dicts.

chipx86chipx86

When you have a string that needs a single quote, it's best to just use double quotes for the outer …

chipx86chipx86

For a lot of our customers, /admin/... won't be a valid URL. That's because Review Board doesn't always live at …

chipx86chipx86

No blank line here.

chipx86chipx86

This is missing "Returns" and "Args".

chipx86chipx86

While we don't always get it right, we generally aim to keep dictionary keys in alphabetical order. This helps with …

chipx86chipx86

This needs to return a dictionary, and can't return a RequestContext. You could in Django 1.6, but 1.11 doesn't allow …

chipx86chipx86

This should be rb/css/defs.less.

chipx86chipx86

Make sure you follow the layout of rules that we use in the other CSS Component files, like page-sidebar.less. There's …

chipx86chipx86

We use two space indentation in CSS.

chipx86chipx86

We don't want to leak CSS-level presentational rules into the HTML. The idea is that the HTML should be semantic. …

chipx86chipx86

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.

chipx86chipx86

Missing a trailig comma here. We always want this on lists of items, so that adding a new line doesn't …

chipx86chipx86

Maybe indenting this line will improve the readability?

LittleGreyLittleGrey

This can end up overriding other onbeforeunload event handlers, which is dangerous. Instead of going this route, we should just …

chipx86chipx86

Should be /**

chipx86chipx86

Is this line missing a colon at the end?

LittleGreyLittleGrey

No space on either side of the =. This is the case with argument defaults.

chipx86chipx86

ES6 JavaScript should use let or const, rather than var.

chipx86chipx86

It's always best to avoid any repeated queries like this. Always fetch the entry you want first and then operate …

chipx86chipx86

I mentioned this a bit up above, but on top of the "don't inject CSS rules as class names" bit, …

chipx86chipx86

This line could be deleted.

LittleGreyLittleGrey

These are indented 1 space too far.

chipx86chipx86

Some notes about indentation here. We use one space indentation for all HTML. This helps to avoid HTML quickly reaching …

chipx86chipx86

Rather than being here, what you'll need to do is put this into the governing PageView class for the page. …

chipx86chipx86

This is specific to the admin UI, so it shouldn't be in the base template. Instead, I think you'll want …

chipx86chipx86

F401 'reviewboard.get_manual_url' imported but unused

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

F821 undefined name 'manual_url'

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

F841 local variable 'request' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'usersCount'

reviewbotreviewbot

F821 undefined name 'reviewGroupsCount'

reviewbotreviewbot

F821 undefined name 'defaultReviewersCount'

reviewbotreviewbot

F821 undefined name 'repositoryCount'

reviewbotreviewbot

Col: 2 Missing semicolon.

reviewbotreviewbot

Docstrings use 4 space indentation.

chipx86chipx86

This should be bool in Python.

chipx86chipx86

This isn't a list of objects, but rather dictionaries. However, we cover what the type is in the Returns block, …

chipx86chipx86

No need for the blank line here.

chipx86chipx86

No \ needed for the single quote, since we're using double quotes for the string now.

chipx86chipx86

Rather than call get_manual_url() over and over, let's call it once at the top and re-use a variable for all …

chipx86chipx86

No \ needed for the single quote.

chipx86chipx86

The %s/ should just be %s, so we don't end up with a double-slash.

chipx86chipx86

No \ needed for the single quote.

chipx86chipx86

This can be one statement.

chipx86chipx86

This can be one statement.

chipx86chipx86

Make sure you're following the pattern we use for other CSS Components. There's a specific documentation and organizational structure we …

chipx86chipx86

This should describe what the page is response for by default.

chipx86chipx86

Blank line between these.

chipx86chipx86

We should avoid using this.options (planning to phase it out), and instead implement an initialize that stores the provided options …

chipx86chipx86

No blank line needed here.

chipx86chipx86

This should go directly above the declaration of the view, or doc generators won't see it.

chipx86chipx86

This should probably live on the view itself. Though, I'd like to see a design that doesn't require this. The …

chipx86chipx86

We should be using CSS component classes, not IDs, for any elements in the banner, and referencing those here.

chipx86chipx86

Remember to use single quotes instead of double quotes unless the content includes a single quote. Same below.

chipx86chipx86

Blank line between these.

chipx86chipx86

There's indentation errors here. Remember to keep the {% unindented and to do indentation within the tag. Also, any {% …

chipx86chipx86

Blank line after {% load %}.

chipx86chipx86

Missing quotes around {{slide.subtitle_url}}

chipx86chipx86

Missing quotes around {{slide.doc_url}}

chipx86chipx86

As discussed above, we'll want to use CSS classes differently here. We probably don't really need to list these as …

chipx86chipx86

There's an exta space before endblock here.

chipx86chipx86

There's an extra space before block here.

chipx86chipx86

Col: 7 Missing semicolon.

reviewbotreviewbot

Col: 67 Missing semicolon.

reviewbotreviewbot

Col: 39 Missing semicolon.

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

The children of "Args" need to be indented 4 more spaces.

chipx86chipx86

Super small thing, but just for consistency, we normally format these as: return json_dumps([ serialize_.... for .... ]) I think, …

chipx86chipx86

This will need to document options. We have a special way we do this. Grep around the codebase for Option …

chipx86chipx86

Since this will only be needed internally, let's name this _setupBannerAttrs, so it's clear it's not something intended for public …

chipx86chipx86

This should document each option using Option Args:.

chipx86chipx86

Each one of these calls is expensive, since it'll need to traverse the DOM. We also then re-perform these in …

chipx86chipx86

This is where anything involving the DOM or browser should go. That includes cookie setup and DOM lookups/event handling.

chipx86chipx86

All comments should be full sentences, with proper punctuation and capitalization. They should also be more explicit. For instance, what …

chipx86chipx86

jQuery can simplify this for us. Iteration through elements should be using: $slides.each(index => { ... });

chipx86chipx86

You can combine these: .removeClass('-is-completed -is-empty -is-optional') This will reduce the parsing overhead needed when rebuilding the class list, since …

chipx86chipx86

This should be formatted such that all arguments align, like: $.cookie( '...', JSON.stringify(...), { ... });

chipx86chipx86

Let's work rbadmin into the beginning of this, so the cookie is namespaced. Since we repeat the name several times, …

chipx86chipx86

We should limit the path to the admin UI, so it's not part of any traffic going to the normal …

chipx86chipx86

Doc comment summaries must be limited to one line. Subsequent information can go in following paragraphs.

chipx86chipx86

If we're removing the element, we also need to remove the view. You can use this.remove().

chipx86chipx86

Several of these statements can be combined easily.

chipx86chipx86

"previous"

chipx86chipx86

Space after <%-.

chipx86chipx86

Indentation levels look to be incorrect, probably on the <div>.

chipx86chipx86

Allowing any code to use cookies during tests (or specifically modifying cookies during tests) can result in unexpected issues when …

chipx86chipx86

This should probably be passed directly to the view. Even if not, I don't think other code is referencing banner, …

chipx86chipx86

It looks like this is only used within this function, and just when setting up the view, so let's remove …

chipx86chipx86

As mentioned above in the implementation, we'll want to store the queried slides directly on the view to avoid repeat …

chipx86chipx86

For this, you'll want $slides.eq(0), which will return a jQuery-wrapped object.

chipx86chipx86

Ideally we'd use expect(...).toHaveClass('...') That does operate on a DOM element, rather than a jQuery-wrapped element, just FYI.

chipx86chipx86

If this is a direct child, we'd want $slide0.children('.rb-c-setup-banner__header')

chipx86chipx86

These would be best using toHaveClass.

chipx86chipx86

These would be updated to use stored variables on the view. It's okay if those variables are "private" (leading underscore). …

chipx86chipx86

This is too long. The function() will need to go on the next line. Same with others below.

chipx86chipx86

Multi-line comments must use the following form: /** * ... */ They also must be proper sentences (capitalization, punctuation). Same …

chipx86chipx86

These should be operating based on the slides (you can use .filter(...) when working off an existing list of jQuery-wrapped …

chipx86chipx86

These tests have a lot of variable definitions for things that are only checked once. It's best to flatten these, …

chipx86chipx86

Normally I'd say just combine these statements, but this is another opportunity to use a pre-calculated variable on the view, …

chipx86chipx86

These are indented 1 too many spaces. This also needs to call {{block.super}} as the first thing.

chipx86chipx86

I might be missing it, but I don't see anything that uses tags from rbadmintags.

chipx86chipx86

Col: 76 Missing semicolon.

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

hxqlin
hxqlin
hxqlin
hxqlin
  1. 
      
  2. Some thinking points for reviewers:

    1. Should authentication settings and inviting team members be combined? From the documentation, they're related.
    2. 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.
    3. 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.
    4. 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?
    5. It feels like there's a lot of slides. Do we need to cut down and if so, where?
    6. 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?)
    1. Hi Hannah, the banner looks really good!

      I do have some feedback to share:

      1. Like Christian mentioned on Slack, it would be nice to see how many steps are in the banner, as well as to see what step the user is currently on. This will give users an idea of where they are so that they won't feel lost.

      2. Also like Christian mentioned, I think it would be better having the close button at the top right. Also, keep in mind that if the user accidentally clicks Dismiss instead of Next, they'll end up closing the banner :(

      3. I'm not sure about having "You're almost ready to start using Review Board!" appearing after the second recommended step. I think it would be cool to mention this in step 5 instead.

      4. About your question for the fixed height for the description, I think that would be nice to do to ensure the design stays consistent (assuming the descriptions are not long and will for sure not overflow off the container :P)

      5. I think it would be nice to keep the logging step, however I can also see that it might be confusing if the user already has logging enabled. Perhaps we can do a check for whether or not logging is enabled, and then hide or checkmark the step if it is enabled? (Just an idea; don't feel obligated to follow it :P)

      6. About review groups and default users, would it be possible to mention both as one step? Not sure though if all the information can fit into the banner (unless we find a way to make a short, yet informative description).

    2. Hi Hannah, the video looks great! 👍

      1. [For question 6] I also think we should fix the height for the description as Katherine mentioned above. I think you may give the container a fixed height and may use !important in css to address this is important.
      2. I think it would be great if we are able to restart this guidline after the user close/dismiss the guildline. I think sometime, as users, they may want to go over again after using the app a while. This is just my thought. :P
      3. [For question 5] I think we can split it into 2 main sections (one is set-up steps, another is recommendations), by changing the background colors. It will tell users that the following slides referring our recommendations.
      4. [For question 2] Instead of checkmarks, we can definitely show how many steps we left. But my ideas is to show numbers(for set-up steps) and some dots buttons (for recommendations) at the center of the container (top or bottom). The numbers and dots buttons are clikable and can jump to corresponding pages. For the current stages, we may have:
        1 2 3 4 5 * * * * *
        I know it may not be consistent, but I do think have 10 steps for the guidline which would be too much. I think if we use numbers and dots buttons, we can tell users which parts would be slightly more important than another.
  3. 
      
hxqlin
hxqlin
hxqlin
Review request changed
Change Summary:

Implement ability to toggle setting to hide/show the banner and add checkmarks for viewed slides in the pagination dots.

Commit:
15e71e57723df069b2b685c46d428556cb09114d
01739a3a9240798466f57e67e715621c86741f28

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

hxqlin
hxqlin
hxqlin
Review request changed
Change Summary:

Refactor template; separated review groups & default reviewers slides.

Commit:
11512076e5255a29ac4f7110ff48dda1e209c0d7
c679c3598fcce8fdb4621d00f5c8c01bbac72af5
Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

  2. Show all issues

    Maybe indenting this line will improve the readability?

    1. What she has here is the correct way of representing a return type. The docs parser will have problems if she indents.

  3. Show all issues

    Is this line missing a colon at the end?

  4. Show all issues

    This line could be deleted.

  5. 
      
chipx86
  1. 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:

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

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

  2. reviewboard/admin/support.py (Diff revision 9)
     
     
    Show all issues

    These arguments aren't actually used anywhere.

  3. reviewboard/admin/support.py (Diff revision 9)
     
     
    Show all issues

    The older functions in this file aren't following modern doc standards, but this should be updated to include any relevant sections, like "Returns".

  4. Show all issues

    These should be listed in alphabetical order.

  5. Show all issues

    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.

  6. Show all issues

    This shouldn't be needed. The function can look this up itself through reviewboard.get_manual_url().

  7. Show all issues

    These are dicts.

  8. Show all issues

    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.

  9. Show all issues

    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.

    1. I'm stuck on /admin/security/ and the ones similar to /admin/settings/*. I looked through the Django Admin documentation but I only found info on reverse, like this: https://docs.djangoproject.com/en/3.0/topics/http/urls/#topics-http-reversing-url-namespaces. Am I look in the right place?

    2. Nvm, figured it out.

  10. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9)
     
     
     
     
    Show all issues

    No blank line here.

  11. Show all issues

    This is missing "Returns" and "Args".

  12. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    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.

  13. Show all issues

    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.

    1. should it return request_context alone or both request and request_context? also, why is it that other functions in the file still return ReturnContext?

    2. Ack, I'm sorry I didn't get to you sooner. This week has been rough for me.

      Excellent question, and I would have expected those to completely break. However, it doesn't, and the reason it doesn't is because of RequestContext acting like a dict. I just dug into the code, and here's what's going on.

      Officially, @inclusion_tag expects a dict result. It's documented this way, and coded this way. But RequestContext conforms to the dict interface, and thus gets consumed as a dict. But it's unnecessary slow for these purposes.

      RequestContext is a specialization of Context (which is what ultimately acts like dict). Context manages a stack of dicts that can be pushed/popped. This is useful for scoping of variables within a template tag. RequestContext specializes Context to inject a bunch of additional variables from the request and also, when requested, from all the context processors (the context_processors.py files in different modules).

      When you consume a Context/RequestContext as a dict, it does the work of collapsing all that down. This means constructing a new dict that contains the contents of each dict owned by the Context (looping through the entire stack and merging them all in). This is, of course, slower than just using a flat dict in the first place.

      So while RequestContext does still work, we should still avoid it because it's slower, it contains more variables than we probably want in the template, and it could conceivably break down the road (though I'm guessing now that that's unlikely, unless they change the code to explicitly check for this).

      Now, where would you want to use RequestContext? Well, in a main template, for sure, because that template or a parent template or a template tag used in any of those templates might have a need for some registered context processor variable from some place. These main templates, usually consumed by a view directly, need to have this flexibility.

      In our case, we know exactly what's going to be used by that included template, so we're in a position to specify the explicit variables we care about. The added benefit is we don't risk collision with other variables from context processors.

      Hope that makes sense! If not, I'll delve into this some more. Django templating machinery is complex :)

  14. Show all issues

    This should be rb/css/defs.less.

  15. reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9)
     
     
     
     
     
    Show all issues

    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.

  16. Show all issues

    We use two space indentation in CSS.

  17. reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    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.

  18. Show all issues

    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.

  19. Show all issues

    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.

  20. Show all issues

    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.

  21. Show all issues

    Should be /**

  22. Show all issues

    No space on either side of the =. This is the case with argument defaults.

  23. Show all issues

    ES6 JavaScript should use let or const, rather than var.

  24. Show all issues

    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.

  25. reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 9)
     
     
     
     
     
     
     
    Show all issues

    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.

  26. reviewboard/templates/admin/base_site.html (Diff revision 9)
     
     
     
     
    Show all issues

    These are indented 1 space too far.

  27. reviewboard/templates/admin/setup_banner.html (Diff revision 9)
     
     
     
     
    Show all issues

    Some notes about indentation here.

    1. We use one space indentation for all HTML. This helps to avoid HTML quickly reaching the wrapping points in editors.
    2. 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.

    1. interesting, hadn't heard of that before!

    2. We might have something somewhere in Notion that talks about it, but it's not prominent. Been meaning to really clarify this, because it's not obvious (and every Django project does something different here). We've settled on this approach to minimize whitespace going to the client, and to keep any HTML output somewhat consistent.

  28. reviewboard/templates/admin/setup_banner.html (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

    1. Does changing all the pages to extend the new RB.Admin.PageView apply to those that extend Backbone.View as well? (There also some that extend other things, like Djblets... that I think would stay as is?)

    2. Only for the administration UI pages that subclass RB.PageView. I don't think we have many of them. Which are you seeing that subclass Djblets?

    3. The full class name is Djblets.RelatedObjectSelectorView - RelatedGroupSelectorView, RelatedRepoSelectorView, and RelatedUserSelectorView all extend it

  29. reviewboard/templates/base.html (Diff revision 9)
     
     
    Show all issues

    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.

    1. I'm not sure if I moved it to the right place - I put it inside the navbar_post block but that's still in the base template, isn't it?

    2. Right, you wouldn't want to put it in the base template. What I mean is that the admin UI's base template can override {% block navbar_post %} and put its own {% block setup_banner %} inside of that block.

  30. 
      
hxqlin
Review request changed
Change Summary:

Fix some issues from review

Commit:
4dab5d065d47da2b94d56cd4ca493de1dfa3adeb
162f7d0ebdba6027f33a9f16793f7d19f7ca041a

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
Review request changed
Change Summary:

Continue fixing issues from review

Commit:
162f7d0ebdba6027f33a9f16793f7d19f7ca041a
7381b546c3baaa59ac27bfc097f29af207a433ed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
Review request changed
Change Summary:

Fix more issues from review - admin pages currently don't display properly after the addition of RB.Admin.PageView

Commit:
7381b546c3baaa59ac27bfc097f29af207a433ed
198a6e045d547a227e8924aa6db4bb333d98bbfd

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
Review request changed
Change Summary:

Fix rendering issues

Commit:
198a6e045d547a227e8924aa6db4bb333d98bbfd
b6dc180f36f4a863a843ee8d052a5b34568dda67

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
hxqlin
hxqlin
Review request changed
Change Summary:

Provide model attributes to setup banner view

Commit:
9460874ad71919503145bc478609e7ecbdfc08dd
1f39fc4ebd0a3469daad2a26c2f434caa9eed4bf

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
hxqlin
Review request changed
Change Summary:

Updated icons

Description:
   

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. Once a slide is viewed, the corresponding
~   dot becomes checked. For the slide currently being viewed, the dot is
~   filled in.

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

   
   

Asana link: https://app.asana.com/0/1157742969496494/1156047935486230/f

Commit:
bc455106213f640f090b2f06d9a69cf01190e6d9
f007458fa6bcaa3cf1d4a47387372c978fd76e92
Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

hxqlin
chipx86
  1. 
      
  2. Show all issues

    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.

    1. Added the depends on links to this review request and will leave this issue open as a reminder to revert those changes in this review request once the separate review requests are landed

  3. reviewboard/admin/support.py (Diff revision 19)
     
     
     
     
    Show all issues

    Docstrings use 4 space indentation.

  4. reviewboard/admin/support.py (Diff revision 19)
     
     
    Show all issues

    This should be bool in Python.

  5. Show all issues

    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.

  6. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19)
     
     
     
     
    Show all issues

    No need for the blank line here.

  7. Show all issues

    No \ needed for the single quote, since we're using double quotes for the string now.

  8. Show all issues

    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.

  9. Show all issues

    No \ needed for the single quote.

  10. Show all issues

    The %s/ should just be %s, so we don't end up with a double-slash.

  11. Show all issues

    No \ needed for the single quote.

  12. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19)
     
     
     
     
     
     
     
    Show all issues

    This can be one statement.

  13. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 19)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This can be one statement.

  14. reviewboard/static/rb/css/ui/admin/setup-banner.less (Diff revision 19)
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  15. Show all issues

    This should describe what the page is response for by default.

  16. Show all issues

    Blank line between these.

  17. Show all issues

    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.

  18. Show all issues

    No blank line needed here.

  19. reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 19)
     
     
     
     
     
     
     
     
    Show all issues

    This should go directly above the declaration of the view, or doc generators won't see it.

  20. Show all issues

    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.

  21. Show all issues

    We should be using CSS component classes, not IDs, for any elements in the banner, and referencing those here.

  22. Show all issues

    Remember to use single quotes instead of double quotes unless the content includes a single quote. Same below.

  23. reviewboard/templates/admin/base_site.html (Diff revision 19)
     
     
     
    Show all issues

    Blank line between these.

  24. reviewboard/templates/admin/base_site.html (Diff revision 19)
     
     
     
     
    Show all issues

    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.

  25. reviewboard/templates/admin/setup_banner.html (Diff revision 19)
     
     
     
    Show all issues

    Blank line after {% load %}.

  26. Show all issues

    Missing quotes around {{slide.subtitle_url}}

  27. Show all issues

    Missing quotes around {{slide.doc_url}}

  28. reviewboard/templates/admin/setup_banner.html (Diff revision 19)
     
     
     
     
     
     
    Show all issues

    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

  29. reviewboard/templates/base.html (Diff revision 19)
     
     
    Show all issues

    There's an exta space before endblock here.

  30. reviewboard/templates/base.html (Diff revision 19)
     
     
    Show all issues

    There's an extra space before block here.

  31. 
      
hxqlin
Review request changed
Change Summary:

Add tests and do some code cleanup

Testing Done:
~  

Manual testing only so far; will write unit & integration tests once

~   the component's behavior and implementation are finalized.

  ~

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 buttons

  +
  +

Manual 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

Commit:
a67636df060358ec24aabdf4bcd8a42207d1f11b
90e47dee6e8f390da36f55e23992bc007b766828

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

hxqlin
hxqlin
hxqlin
hxqlin
Review request changed
Change Summary:

Fix how attributes are passed to JS view from Python

Commit:
e60255f77719662a3876430bb9c3e5e43414aec1
7c85e8bc4522c3c645d17a683c8a24d720c33f0f

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
hxqlin
hxqlin
hxqlin
chipx86
  1. 
      
  2. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 27)
     
     
     
     
    Show all issues

    The children of "Args" need to be indented 4 more spaces.

  3. reviewboard/admin/templatetags/rbadmintags.py (Diff revision 27)
     
     
     
    Show all issues

    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:

    1. When a function is defined inline, it will actually be re-created every time the containing function is called. There's overhead to this.
    2. Generators also add additional overhead. Not much, but they're going to be slower than manually building.
    3. 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)
    
  4. reviewboard/static/rb/js/admin/views/pageView.es6.js (Diff revision 27)
     
     
     
     
     
    Show all issues

    This will need to document options. We have a special way we do this. Grep around the codebase for Option Args:.

    1. Not quite sure if I did this right - should I also be documenting model.attributes here?

  5. Show all issues

    Since this will only be needed internally, let's name this _setupBannerAttrs, so it's clear it's not something intended for public use.

  6. Show all issues

    This should document each option using Option Args:.

  7. reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27)
     
     
     
     
     
     
     
     
     
    Show all issues

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

  8. Show all issues

    This is where anything involving the DOM or browser should go. That includes cookie setup and DOM lookups/event handling.

  9. Show all issues

    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?

  10. reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27)
     
     
     
     
     
     
     
    Show all issues

    jQuery can simplify this for us. Iteration through elements should be using:

    $slides.each(index => {
        ...
    });
    
  11. Show all issues

    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.

  12. reviewboard/static/rb/js/admin/views/setupBannerView.es6.js (Diff revision 27)
     
     
     
     
     
     
     
     
     
    Show all issues

    This should be formatted such that all arguments align, like:

    $.cookie(
        '...',
        JSON.stringify(...),
        {
            ...
        });
    
  13. Show all issues

    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.

    1. Not sure if the naming is right - I used an underscore like rbadmin_setupbannerstate

  14. Show all issues

    We should limit the path to the admin UI, so it's not part of any traffic going to the normal user-facing pages.

  15. Show all issues

    Doc comment summaries must be limited to one line. Subsequent information can go in following paragraphs.

  16. Show all issues

    If we're removing the element, we also need to remove the view. You can use this.remove().

  17. Show all issues

    Several of these statements can be combined easily.

  18. Show all issues

    "previous"

  19. Show all issues

    Space after <%-.

  20. Show all issues

    Indentation levels look to be incorrect, probably on the <div>.

  21. Show all issues

    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.

  22. Show all issues

    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 this beforeEach().

    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.

  23. reviewboard/static/rb/js/admin/views/tests/setupBannerViewTests.es6.js (Diff revision 27)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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:

    [
        {
            ...,
        },
        {
            ...,
        },
        ...,
    ]
    
  24. Show all issues

    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.

  25. Show all issues

    For this, you'll want $slides.eq(0), which will return a jQuery-wrapped object.

  26. Show all issues

    Ideally we'd use

    expect(...).toHaveClass('...')
    

    That does operate on a DOM element, rather than a jQuery-wrapped element, just FYI.

  27. Show all issues

    If this is a direct child, we'd want $slide0.children('.rb-c-setup-banner__header')

  28. Show all issues

    These would be best using toHaveClass.

  29. Show all issues

    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.

  30. Show all issues

    This is too long. The function() will need to go on the next line.

    Same with others below.

  31. Show all issues

    Multi-line comments must use the following form:

    /**
     * ...
     */
    

    They also must be proper sentences (capitalization, punctuation).

    Same with others.

  32. Show all issues

    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.

  33. Show all issues

    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);
    
  34. Show all issues

    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.

  35. reviewboard/templates/admin/base_site.html (Diff revision 27)
     
     
     
     
    Show all issues

    These are indented 1 too many spaces.

    This also needs to call {{block.super}} as the first thing.

  36. Show all issues

    I might be missing it, but I don't see anything that uses tags from rbadmintags.

  37. 
      
hxqlin
Review request changed
Change Summary:

Fix issues from review

Commit:
2c8bdf19e7639c328c79af460f2a61ac643c12f0
f3cc333f0ff6750f9b6a56d866431397819ccc14

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

hxqlin
Review request changed