Skip to content

Refactor sidebar logic for more predictable behavior

username-removed-636429 requested to merge 19183-refactor-sidebar-js into master

What does this MR do?

Fixes a few bugs with the sidebar and "pin" functionality:

  1. Pinned state would get reset when loading a page with a viewport smaller than 1024px (#19183 (closed))

  2. Toggle buttons could occasionally end up in an invalid state in which the toggle is hidden from view at the same time the sidebar is collapsed. 2016-09-03-09.39.07

  3. Clicking outside the sidebar to trigger 'collapse' when below the 1024px breakpoint would work only if not pinned, even though pin status should only effect the sidebar above the 1024px breakpoint.

  4. Code confusing with no single source of truth for the state of the sidebar. Sometimes pinned state is inferred from the cookie, sometimes from the DOM. Code to handle these functions was confusingly split across both sidebar.js and application.js for no apparent reason.

I've created a singleton ES6 class to handle the sidebar DOM manipulations, using the properties isExpanded and isPinned as the canonical state and providing a renderState method to sync the DOM with this state whenever it needs updating. This avoids the possibility of invalid states caused by reliance on jQuery toggleClass() methods and makes the code much more readable/maintainable.

Are there points in the code the reviewer needs to double check?

It is a substantial rewrite, so I could use another set of eyes to make sure nothing was left behind from the original implementation.

Why was this MR needed?

I initially intended to just fix #19183 (closed) by modifying the code in place, but it proved to be a difficult mess and rather than add to the technical debt it made sense to write a more readable implementation of the sidebar functionality.

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #19183 (closed)

Merge request reports