Skip to content
Snippets Groups Projects

Fix SCSS interpolation on MR sticky sidebar height and margin-bottom

Merged Scott de Jonge requested to merge mr-sticky-sidebar-height into master
All threads resolved!

What does this MR do and why?

The height and margin-bottom properties for the moved MR sticky sidebar weren't being computed from SCSS variables to CSS values.

The sidebar was previously using a negative margin-bottom to compensate for $content-wrapper-padding (100px) at the bottom of the viewport.

The height property allows the sidebar to be scrolled independently of the viewport at smaller heights (or when sidebar content extends past the viewport height).

SCSS variables need to be interpolated within calc() blocks for the values to be computed

Before:

@media (min-width: 992px) {
  .right-sidebar.right-sidebar-expanded .issuable-sidebar.is-merge-request .issuable-context-form {
    height: calc(calc(100vh - calc(var(--header-height, 48px) + calc(var(--system-header-height) + var(--performance-bar-height)) + var(--top-bar-height)) - var(--system-footer-height)) - 76px - var(--mr-review-bar-height) - $content-wrapper-padding);
    margin-bottom: calc((var(--header-height, 48px) + $issue-sticky-header-height) * -1);
  }
}

After:

@media (min-width: 992px) {
  .right-sidebar.right-sidebar-expanded .issuable-sidebar.is-merge-request .issuable-context-form {
    height: calc(calc(100vh - calc(var(--header-height, 48px) + calc(var(--system-header-height) + var(--performance-bar-height)) + var(--top-bar-height)) - var(--system-footer-height)) - 72px - var(--mr-review-bar-height));
    margin-bottom: calc((100px * -1) + var(--mr-review-bar-height));
  }
}

Changes

  • Update local $issue-sticky-header-height variable to use global $mr-sticky-header-height variable
  • Remove $content-wrapper-padding from height calculation
  • Update margin-bottom to compensate for bottom padding

Screenshots or screen recordings

Before After
CleanShot_2023-06-06_at_12.19.09 CleanShot_2023-06-06_at_14.31.30

How to set up and validate locally

  1. Enable Feature.enable(:moved_mr_sidebar)
  2. View MR page e.g. http://127.0.0.1:3000/flightjs/Flight/-/merge_requests/4
  3. Scroll to bottom of page
    1. Right sidebar should not scroll up
  4. Decrease height so sidebar contents are longer than viewport height
    1. Scroll inside sidebar contents without scrolling viewport

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 Scott de Jonge

Merge request reports

Merged results pipeline #893120899 passed

Pipeline: E2E GDK

#893127494

    Pipeline: GitLab

    #893128529

      Pipeline: GitLab

      #893125334

        Merged results pipeline passed for 3f1e8201

        Test coverage 67.96% (1.34%) from 2 jobs
        Approved by

        Merged by Peter HegmanPeter Hegman 1 year ago (Jun 8, 2023 1:29am UTC)

        Merge details

        • Changes merged into master with 36bcff26 (commits were squashed).
        • Deleted the source branch.
        • Auto-merge enabled

        Pipeline #893180084 passed

        Pipeline passed for 36bcff26 on master

        Test coverage 66.65% (1.34%) from 2 jobs
        10 environments impacted.

        Activity

        Filter activity
        • Approvals
        • Assignees & reviewers
        • Comments (from bots)
        • Comments (from users)
        • Commits & branches
        • Edits
        • Labels
        • Lock status
        • Mentions
        • Merge request status
        • Tracking
      • Scott de Jonge
      • Scott de Jonge requested review from @slashmanov

        requested review from @slashmanov

      • Contributor

        Bundle size analysis [beta]

        This compares changes in bundle size for entry points between the commits a689aeeb and 305a1d6e

        :sparkles: Special assets

        Entrypoint / Name Size before Size after Diff Diff in percent
        average 4.14 MB 4.14 MB - 0.0 %
        mainChunk 2.99 MB 2.99 MB - 0.0 %

        Note: We do not have exact data for a689aeeb. So we have used data from: 27f59b50.
        The intended commit has no webpack pipeline, so we chose the last commit with one before it.

        Please look at the full report for more details


        Read more about how this report works.

        Generated by :no_entry_sign: Danger

      • Contributor

        Allure report

        allure-report-publisher generated test report!

        e2e-test-on-gdk: :exclamation: test report for 305a1d6e

        expand test summary
        +-----------------------------------------------------------------------+
        |                            suites summary                             |
        +------------------+--------+--------+---------+-------+-------+--------+
        |                  | passed | failed | skipped | flaky | total | result |
        +------------------+--------+--------+---------+-------+-------+--------+
        | Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
        | Plan             | 4      | 0      | 0       | 0     | 4     | ✅     |
        | Create           | 8      | 0      | 1       | 0     | 9     | ✅     |
        | Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
        | Data Stores      | 2      | 0      | 0       | 1     | 2     | ❗     |
        | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
        | Manage           | 1      | 0      | 0       | 0     | 1     | ✅     |
        +------------------+--------+--------+---------+-------+-------+--------+
        | Total            | 21     | 0      | 2       | 1     | 23    | ❗     |
        +------------------+--------+--------+---------+-------+-------+--------+

        e2e-review-qa: :exclamation: test report for 305a1d6e

        expand test summary
        +-----------------------------------------------------------------------+
        |                            suites summary                             |
        +------------------+--------+--------+---------+-------+-------+--------+
        |                  | passed | failed | skipped | flaky | total | result |
        +------------------+--------+--------+---------+-------+-------+--------+
        | Plan             | 3      | 0      | 1       | 0     | 4     | ✅     |
        | Manage           | 8      | 0      | 3       | 0     | 11    | ✅     |
        | Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
        | Create           | 8      | 0      | 1       | 0     | 9     | ✅     |
        | Data Stores      | 2      | 0      | 0       | 1     | 2     | ❗     |
        | Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
        | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
        +------------------+--------+--------+---------+-------+-------+--------+
        | Total            | 27     | 0      | 6       | 1     | 33    | ❗     |
        +------------------+--------+--------+---------+-------+-------+--------+
      • Scott de Jonge added 238 commits

        added 238 commits

        • 21ab32b6...6fb52274 - 236 commits from branch master
        • cbe0ca99 - Fix SCSS interpolation on MR sticky sidebar height and margin-bottom
        • 305a1d6e - Fix margin-bottom for MR review bar offset

        Compare with previous version

      • Scott de Jonge changed the description

        changed the description

      • Annabel Dunstone Gray approved this merge request

        approved this merge request

      • removed review request for @annabeldunstone

      • :wave: @annabeldunstone, thanks for approving this merge request.

        This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

        For more info, please refer to the following links:

      • Stanislav Lashmanov approved this merge request

        approved this merge request

      • Stanislav Lashmanov requested review from @peterhegman and removed review request for @slashmanov

        requested review from @peterhegman and removed review request for @slashmanov

      • Peter Hegman approved this merge request

        approved this merge request

      • Peter Hegman resolved all threads

        resolved all threads

      • Peter Hegman enabled an automatic merge when the pipeline for 3f1e8201 succeeds

        enabled an automatic merge when the pipeline for 3f1e8201 succeeds

      • merged

      • Peter Hegman mentioned in commit 36bcff26

        mentioned in commit 36bcff26

      • added workflowstaging label and removed workflowcanary label

      • Please register or sign in to reply
        Loading