Skip to content

Mass button migration

Mark Florian requested to merge mf-button-migration into master
ℹ️ UX README
Before diving into the review, pick the files you'll be reviewing and add your name in the UX review tracker below.

What does this MR do and why?

This migrates many links that are styled to look like buttons to a new helper, link_button_to.

These migrations were largely done blind, in that a codemod was used to perform them.

Note that a number of the migrated link buttons have existing issues, for instance:

  • many do not have an accessible name;
  • some use a disabled attribute/style, which makes no sense for a link.

This MR does not fix those issues. It is simply about replacing link_to calls with CSS classes applied with a better encapsulated link_button_to helper.

Part of https://gitlab.com/gitlab-com/gitlab-OKRs/-/work_items/2030.

Commit messages

Remove unnecessary gl-button classes

The app/views/shared/issuable/_search_bar.html.haml template renders the deprecated, jQuery-based filtered search (which should eventually be replaced with the GlFilteredSearch Vue component). It's used in places like the Merge Requests list and Issue Analytics.

These buttons should never have had the gl-button class applied to them, since their styles are fully described by the (deprecated) filtered search styles anyway. Due to the existence of the class, they are false positive findings detected by the Pajamas Adoption Scanner. Removing the class will prevent the Scanner from detecting them.

Remove unnecessary gl-btn classes

This class doesn't do anything. These should also not have the .gl-button class, since they're part of dropdown's options, which are styled differently anyway.

Manually rewrite disabled button

Replace pluralize calls with n_

For improved localisation.

Add link_button_to helper

This provides an easier migration path for links that look like buttons than using Pajamas::ButtonComponent itself. This is because the method signature is much more similar to link_to than Pajamas::ButtonComponent.

It's also slightly less verbose, which seems reasonable for such a common use case.

Mass button migration

Executed with:

comby \
    -config ~/dev/pajamas-adoption-scanner/comby/button.toml \
    -custom-matcher ~/dev/pajamas-adoption-scanner/comby/haml.json \
    -matcher .generic -in-place \
    -f {ee/,}app/views/**/*.haml

Using the button-migration branch of the Pajamas Adoption Scanner: https://gitlab.com/gitlab-org/frontend/pajamas-adoption-scanner/-/commits/button-migration

Button post-migration manual fixes

  • Translate untranslated button text
  • Use double quotes for interpolation of classes
  • Migrate some link buttons in the vicinity not caught by migration
  • Clean up html_options hashes, empty class attributes
  • Unwrap unnecessary span element surrounding link
  • Use keyword arguments rather than an explicit hash argument

Fix custom icon rendering

The Gitea logo isn't available as a regular icon, so cannot be set via the icon argument. It must be handled specially.

The styling here ensures that the icon and the text are displayed on the same line.

Replace .sr-only with aria-label

While the screen-reader-only label seems redundant given the link already has accessible text, it exists so that the confirmation modal picks up the text for its confirm button.

For some reason, it explicitly looks for screen-reader-only text. The original MR does not explain why. There is an issue to explore changing this.

Why switch from .sr-only to aria-label:

  • To be consistent with link button above it.
  • The sr-only class is from Bootstrap, and generally we're trying to move away from that.

Rename parameters and call block with yield

Move helper to more appropriate file

Remove Gitea icon button hack

It turns out the Gitea logo is in our sprite icon set, so the icon option can be used.

Screenshots or screen recordings

In theory, there should be no visual or behavioral differences.

How to set up and validate locally

Because this changes buttons in various places across the app, there's no single process to validate this. Instead, I can provide some heuristics:

  1. Choose a file in the diff.
  2. Based on its name, or the user-facing strings inside it, guess where it appears in the app.
  3. Either:
    • try to perform the actions necessary to render the button(s) in the file, or
    • change the code to force the button to render.
  4. Verify the button looks correct.
  5. Verify clicking the button has the correct behavior. Note: this might not always be a simple page load! Buttons which have, e.g., method: :post or similar on them might have other behavior.

UX review tracker

  1. Once you have picked the files you will review, add your username next to them (or to the containing folder) so that everyone's aware of who is reviewing what, and we avoid duplicate efforts.
  2. After you've reviewed them, check them off. Thank you!

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 Mark Florian

Merge request reports