Skip to content
GitLab
Next
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • GitLab FOSS GitLab FOSS
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Locked Files
  • Issues 0
    • Issues 0
    • List
    • Boards
    • Service Desk
    • Milestones
    • Iterations
    • Requirements
  • Merge requests 1
    • Merge requests 1
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Package Registry
    • Container Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Metrics
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • Code review
    • Insights
    • Issue
    • Repository
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Commits
  • Issue Boards
Collapse sidebar
  • GitLab.orgGitLab.org
  • GitLab FOSSGitLab FOSS
  • Merge requests
  • !32198

Resolve "When the branch name is a divider or separator, the project home page cannot display the branch name correctly."

  • Review changes

  • Download
  • Email patches
  • Plain diff
Closed Thomas Randolph requested to merge 65473-when-the-branch-name-is-a-divider-or-separator-the-project-home-page-cannot-display-the-branch-name-correctly into master Aug 26, 2019
  • Overview 52
  • Commits 5
  • Pipelines 39
  • Changes 11

What does this MR do?

This MR fixes an issue where branches named separator or divider don't show up in the project ref switcher, but do cause it to render an actual divider or separator dropdown item.

Problem

The gl_dropdown.js file reads in string primitives for each data item. If these strings match either divider or separator, the file doesn't render the data, and instead renders an actual divider or separator item.

Unfortunately, "separator" and "divider" are much too easy to accidentally conflict with.

Solution

This MR completely refactors the rendering portion(s) of the gl_dropdown.js file.

All dividers and separators must be generated by { "type": ("separator"|"divider") } instead of plaintext strings.

More solution notes from commit c91f915a:


Basic Intent: Allow all branch names without accidentally creating layout or backstage DOM. e.g. a branch named separator should never create a separator li element.

Ideally, there should never be a string that could cause this kind of conflict.

Implementation: All of GitLabDropdown.renderItem is extracted to a standalone module.

To render a divider or separator, consumers must now pass in an object like { "type": "divider" } or { "type": "separator" }

Notable choices:

  • All of the functions have a cyclomatic complexity of 3 or less
    • See: https://en.wikipedia.org/wiki/Cyclomatic_complexity
    • Note the "Correlation to number of defects" section
    • While software complexity may not have a directly causal relationship with defects, less complex software is generally easier to reason about, and may reduce defects. I personally try to maintain complexity of no higher than 3.

Other Notes

There's an epic issue that addresses the double scrollbar problem that occurs when the branch/tag/commit list is scrollable, which is visible in the after screenshot (since the list is now long enough to scroll, barely), or on gitlab.com.

Screenshots

Before After
sep-before sep-after

Does this MR meet the acceptance criteria?

Conformity

  • Changelog entry for user-facing changes, or community contribution. Check the link for other scenarios.
  • Documentation created/updated or follow-up review issue created
  • Code review guidelines
  • Merge request performance guidelines
  • Style guides
  • Database guides
  • Separation of EE specific content

Performance and Testing

  • Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
  • Tested in all supported browsers

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • ~~Label as security and @ mention @gitlab-com/gl-security/appsec~~
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #65473 (moved)

Edited Sep 08, 2019 by Thomas Randolph
Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: 65473-when-the-branch-name-is-a-divider-or-separator-the-project-home-page-cannot-display-the-branch-name-correctly