Skip to content

Create app-specific approval_rules_app.vue

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

approval_rules_app.vue currently works as a catch-all wrapper with some general utilities and reusable components (add/reset buttons, creation drawer/modal).

Because of the differences in the child components, it also contains booleans for determining where it's used. Those are also passed down to child components.

We need to remember that recently it was used in Project Settings and MR edit views, but we already included it on Branch Rule Edit page (MVC, behind feature flag) and will do so on Group Settings page.

Having defined heading level (<h5>) reused in different HTML context results in accessibility violations. This heading level should be different on a Setting page and on an MR edit page. It could be dynamically determined, for example based on isMrEdit and isBranchRulesEdit boolean, but it will further increase the complexity.

I propose getting rid of the component and implementing it application-specific versions in each of the location. What we could gain:

  • removing passing down location flags through multiple levels
  • HTML structure matching the context in which we render the rules
  • the footer template of the card can be used only in the relevant location (MR Edit)
  • the same goes for conditionally fetching rules on mount, when the component is used on Branch Rule Edit page
<div v-if="checkShowResetButton" class="border-bottom py-3 px-3">
  <div class="gl-display-flex">
    <gl-button
      v-if="targetBranch"
      :disabled="isLoading"
      size="small"
      data-testid="reset-to-defaults"
      @click="resetToProjectDefaults"
    >
      {{ __('Reset to project defaults') }}
    </gl-button>
  </div>
</div>

A couple of things to correct in the above code block:

  • border-bottom should be change to a gl- utility class. It should also be a top border. The current class results in a double bottom border at the end of the table, but none that would separate it from the row above. I suspect that the styles were reversed at some point and this section was overlooked due to conditional rendering.
  • this code block includes only the button. The wrapping <div> element with flexbox can be removed and is not needed.
  • It looks like this section could have included more elements in the past. At this point, we have two different conditions used to in the end only show one button. checkShowResetButton method includes all necessary data to decide if the button should render. targetBranch condition on the button is not needed as it's already included in checkShowResetButton.
  • note that having targetBranch defined effectively means that the component is used on the MR Edit page, it's the only place in which we provide and need that information. Which means that this codeblock together with its methods could be used only in ee/app/assets/javascripts/approvals/mr_edit/app.vue

Removing approval_rules_app.vue may seem counterintuitive as it will create code duplication to some extend. But since this component is to be used in 4 different locations, we will get rid of a problem that is customizing something that was supposed to be an abstraction.

Edited by 🤖 GitLab Bot 🤖