Skip to content

Allow focus in reviewers dropdown search field before data loads

Thomas Randolph requested to merge tor/defect/focusable-reviewers-dropdown into master

What does this MR do and why?

Reviewers, Read This Paragraph

For review, you must turn off Show whitespace changes in the Changes tab settings cog ().
This is a fairly well-contained MR which Prettier mangled by adding formatting and indentation that should not be necessary.
More than 1000 lines of code changes listed in this MR are actually only whitespaces changes caused by Prettier.



Why

For #418990 (closed)

Modifies the behavior of the reviewers dropdown in the sidebar to allow for focus in the search/filter input prior to the remote data loading.

When you open the Assignees dropdown on an MR, the search field is immediately focused (so you can start typing to pre-filter the possible assignees).

This doesn't happen with Reviewers, leading to a disjointed experience and captured keyboard commands that don't go to the reviewers dropdown but are instead captured by the page, leading to arbitrary shortcut execution (like opening a new issue).

This solution brings the reviewers dropdown more in line with how the Assignees dropdown works.

Architectural decisions

Given the highly complex state of the Reviewers dropdown, there were essentially three options:

  1. Modify the dropdown behavior to change when and where the loading indicators appear
  2. Manually modify the DOM to fake the experience we want "over top" of the dropdown
  3. Rewrite the entire set of components in a more reusable way
  • The third option (rewrite the components) is the most correct path forward, but it would be a very large undertaking, so it was discarded (for now).
  • The first option is inadvisable since the Reviewers dropdown uses the deprecatedJQueryDropdown component under the hood. Modifying this could lead to many unrelated broken parts of the GitLab UI and it's simply not a good use of time. The component is deprecated, after all!
  • The second option is extremely cumbersome and it still leaves a lot to be desired (for example there would still be a blocking loader over the dropdown, but we'd fake an input on top of that and then feed that input's values into the dropdown when it hid the spinner).

The approach I instead took here was minimal additive-only changes to the deprecated dropdown to add "lifecycle hooks." These should have no effect on other consumers of the deprecated dropdown, since they are completely new (events). Meanwhile, they give our specific consumer (the Reviewers dropdown) the hooks necessary to perform a little work.

With the hooks in place, the Issuable context - the file that initializes the dropdowns in the sidebar - can manage some light touch DOM changes:

  • Adding a CSS class that changes some visibility, sizes, and positioning
  • Moving the spinner to a slightly different DOM tree location

This latter point - where the spinner is moved (and then returned to it's original location!) - is important because it allows the deprecated dropdown to continue operating in exactly the way it did before, while still having a new, more usable display.

So, this is the most minimal set of viable changes from options 1 & 2 while trying to avoid any deep changes to either the deprecated component or the complex DOM tree.

Screenshots or screen recordings

Before After
focusable-reviewers-before focusable-reviewers-after

How to set up and validate locally

  1. Add sleep(10.seconds) to this line
  2. Visit an MR
  3. Open the reviewers edit dropdown, interact with it (filter the list, etc.)

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Thomas Randolph

Merge request reports