Skip to content
Snippets Groups Projects

Add DevopsAdoptionTableCellFlag component

Merged Brandon Labuschagne requested to merge add-devops-adoption-table-cell-flag-component into master
All threads resolved!

What does this MR do?

This component is needed in order to display the adoption status of an item, i.e. true or false - within the Devops Adoption feature.

The only way to test this locally would be to test the change in !47164 (merged).

I've opted to open this separate MR in order to iterate while blocked on other parts of the main MR. This also enables me to produce nice and small, easily reviewable MRs :man_dancing:

The border is $gray-300 and the fill is $indigo-600

Feature flag rollout issue: #271568 (closed)

Screenshots (strongly suggested)

The component is the small circles within the table :sweat_smile:

Screenshot_2020-11-11_at_12.55.57

Enabled Disabled
Screenshot_2020-11-11_at_12.56.06 Screenshot_2020-11-11_at_11.33.02

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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

Related to #271249 (closed)

Edited by Brandon Labuschagne

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • 55fc408f - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • 1 Warning
    :warning:

    featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

    For more information, see:

    1 Message
    :book: CHANGELOG missing:

    If you want to create a changelog entry for GitLab FOSS, run the following:

    bin/changelog -m 47376 "Add DevopsAdoptionTableCellFlag component"

    If you want to create a changelog entry for GitLab EE, run the following instead:

    bin/changelog --ee -m 47376 "Add DevopsAdoptionTableCellFlag component"

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    Reviewer roulette

    Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited, or the chosen person is unavailable.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.

    Category Reviewer Maintainer
    frontend Thomas Randolph (@thomasrandolph) (UTC-7, 9 hours behind @blabuschagne) Andrew Fontaine (@afontaine) (UTC-5, 7 hours behind @blabuschagne)

    If needed, you can retry the danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • @thomasrandolph can you please perform the initial review? :bow:

  • Nick Post approved this merge request

    approved this merge request

  • unassigned @npost

  • Thomas Randolph approved this merge request

    approved this merge request

    • Resolved by Andrew Fontaine

      @blabuschagne

      As a change, I'm happy with the content here!
      Everything seems fine to me!

      I do have one concern, though: this seems generic enough to be re-used anywhere in GitLab where we might want "a little dot that's either filled in or not." Given that it's named specifically for DevopsAdoption and the CSS is specific to one page's bundle, there's a non-trivial chance that someone else at the company might also want "empty or filled dots" and wind up re-implementing this component (probably slightly differently, leading to low design cohesion!).

      So this is the long way around to: Did you consider submitting this as a new Gitlab UI component?

      cc: @npost Is this a pattern we might reuse for other "on/off", "yes/no", etc. indicators?

      This isn't a blocker by any means, but I'd be disappointed if I were to see this same pattern repeated across GitLab and not centralized into one standard BooleanFlag UI component.

      Approving, but I think this should be seriously considered as a GitLab-pattern component, not just a Admin/DevOps Report pattern.

      :grey_question: What is this
      Blocker No
      Concern Level 1-10 1
  • @afontaine could you please maintainer review this? :bow:

  • Brandon Labuschagne changed the description

    changed the description

  • Brandon Labuschagne mentioned in merge request !47491 (merged)

    mentioned in merge request !47491 (merged)

  • Andrew Fontaine resolved all threads

    resolved all threads

  • Andrew Fontaine approved this merge request

    approved this merge request

  • I echo @thomasrandolph's concerns, but it looks like you have a good plan in place @blabuschagne

    Looks great :grin: Setting to MWPS

  • Andrew Fontaine enabled an automatic merge when the pipeline for eb694257 succeeds

    enabled an automatic merge when the pipeline for eb694257 succeeds

  • Andrew Fontaine mentioned in commit 6ecb4aa8

    mentioned in commit 6ecb4aa8

  • added workflowcanary label and removed workflowstaging label

  • added workflowproduction label and removed workflowcanary label

  • Please register or sign in to reply
    Loading