Admin UI blank state banner

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

hxqlin
Review Board
master
10979, 10978
00113ac...
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.

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

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

Loading file attachments...

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

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

chipx86chipx86

Blank line between these.

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

Diff:

Revision 6 (+396 -3)

Show changes

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

Diff:

Revision 8 (+427 -3)

Show changes

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. 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. Is this line missing a colon at the end?

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

    These arguments aren't actually used anywhere.

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

  4. These should be listed in alphabetical order.

  5. 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. This shouldn't be needed. The function can look this up itself through reviewboard.get_manual_url().

  7. These are dicts.

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

    No blank line here.

  11. This is missing "Returns" and "Args".

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

  13. 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. This should be rb/css/defs.less.

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

  16. We use two space indentation in CSS.

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

  18. 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. 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. 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. No space on either side of the =. This is the case with argument defaults.

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

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

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

  25. reviewboard/templates/admin/base_site.html (Diff revision 9)
     
     
     
     

    These are indented 1 space too far.

  26. reviewboard/templates/admin/setup_banner.html (Diff revision 9)
     
     
     
     

    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.

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

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

    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.

  29. 
      
hxqlin
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
Review request changed

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

Diff:

Revision 12 (+501 -10)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
hxqlin
hxqlin
Review request changed

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

Diff:

Revision 18 (+621 -16)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

hxqlin
chipx86
  1. 
      
  2. 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.

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

    Docstrings use 4 space indentation.

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

    This should be bool in Python.

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

    No need for the blank line here.

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

  8. 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. No \ needed for the single quote.

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

  11. No \ needed for the single quote.

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

    This can be one statement.

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

    This can be one statement.

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

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

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

  17. Blank line between these.

  18. No blank line needed here.

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

  20. 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. We should be using CSS component classes, not IDs, for any elements in the banner, and referencing those here.

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

    Blank line between these.

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

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

    Blank line after {% load %}.

  26. Missing quotes around {{slide.subtitle_url}}

  27. Missing quotes around {{slide.doc_url}}

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

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

    There's an exta space before endblock here.

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

    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

Diff:

Revision 20 (+971 -19)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

hxqlin
hxqlin
Review request changed
Loading...