Skip to content
Snippets Groups Projects

Use GlCollapsibleListbox in ci_environments_dropdown.vue

Merged Eugie Limpin requested to merge el-use-collapsible-listbox into master
1 unresolved thread

Resolves #384285 (closed)

What does this MR do and why?

This MR changes the Vue component used to select environment scope of CI environment variables from GlDropdown to GlCollapsibleListbox.

Screenshots or screen recordings

before after
Closed b4_closed closed
Open b4_open open
With selection b4_with_selection with_selection
With search query b4_with_search with_search

Difference in looks

The dropdown is not occupying the whole width like before. This is because GlCollapsibleList doesn't (yet?) support specifying additional classes for the toggle button text (it only supports toggleClass which goes to the button.gl-button-dropdown-toggle).

Why? Screen_Shot_2022-12-07_at_9.56.03_PM

Difference in behavior

The dropdown does not clear the search term when it is closed nor when a new environment is created. This is because GlCollapsibleList does not support programmatically clearing the search input.

before after
Screen_Recording_2022-12-26_at_4.58.01_PM Screen_Recording_2022-12-26_at_5.15.04_PM

How to set up and validate locally

  1. Go to the CI settings page of a project. For example: http://localhost:3000/flightjs/Flight/-/settings/ci_cd
  2. Expand Variables section then click on Add variable
  3. Validate that you can:
    1. Select an existing environment
    2. Search through existing environments
    3. Create a new environment with a search term that doesn't correspond to any existing environment

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 Eugie Limpin

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
  • Eugie Limpin changed milestone to %15.7

    changed milestone to %15.7

  • Eugie Limpin changed title from El use collapsible listbox to Use GlCollapsibleListbox in ci_environments_dropdown.vue

    changed title from El use collapsible listbox to Use GlCollapsibleListbox in ci_environments_dropdown.vue

  • A deleted user added frontend label

    added frontend label

  • Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    frontend Mireya Andres current availability (@mgandres) (UTC+8, same timezone as @eugielimpin) Olena HK. current availability (@ohoral) (UTC+2, 6 hours behind @eugielimpin)
    test for spec/features/* Stanislav Dobrovolschii current availability (@sdobrovolschii) (UTC+1, 7 hours behind @eugielimpin) Maintainer review is optional for test for spec/features/*
    UX Katie Macoy current availability (@katiemacoy) (UTC+13, 5 hours ahead of @eugielimpin) Maintainer review is optional for UX

    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 use the GitLab Review Workload Dashboard to find other available reviewers.

    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, assign them as a reviewer! Danger does not automatically notify them for you.

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

    Generated by :no_entry_sign: Danger

  • Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits e020ee83 and 02f3cb16

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 3.54 MB 3.54 MB - -0.0 %
    mainChunk 1.95 MB 1.95 MB - 0.0 %

    :fearful: Significant Growth: 1

    Expand
    Entrypoint / Name Size before Size after Diff Diff in percent
    pages.groups.settings.ci_cd.show 1.34 MB 1.45 MB +112.25 KB 8.2 %

    :tada: Significant Reduction: 6

    Expand
    Entrypoint / Name Size before Size after Diff Diff in percent
    pages.admin.application_settings 1.34 MB 1.29 MB -48.52 KB -3.5 %
    pages.admin.application_settings.advanced_search 1.34 MB 1.29 MB -48.52 KB -3.5 %
    pages.admin.application_settings.integrations 1.35 MB 1.3 MB -48.52 KB -3.5 %
    pages.admin.application_settings.metrics_and_profiling 1.34 MB 1.29 MB -48.52 KB -3.5 %
    pages.admin.application_settings.reporting 1.43 MB 1.38 MB -48.52 KB -3.3 %
    pages.admin.application_settings.service_usage_data 1.34 MB 1.29 MB -48.52 KB -3.5 %

    Your MR has at least one entrypoint growing significantly (more > 1 KB or 2%). If you write new or extend existing features, this is expected and there is nothing to worry about.

    Please consider pinging someone from the FE Foundations (@leipert, @markrian, @mikegreiling, @ohoral or @pgascouvaillancourt) for review, if you are unsure about the size increase.

    Note: We do not have exact data for e020ee83. So we have used data from: 33e342b7.
    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

  • Allure report

    allure-report-publisher generated test report!

    e2e-review-qa: :exclamation: test report for 02f3cb16

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Create           | 28     | 0      | 1       | 0     | 29    | ✅     |
    | Plan             | 49     | 0      | 1       | 0     | 50    | ✅     |
    | Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Manage           | 34     | 0      | 3       | 1     | 37    | ❗     |
    | Framework sanity | 9      | 0      | 1       | 0     | 10    | ✅     |
    | Govern           | 24     | 0      | 5       | 0     | 29    | ✅     |
    | Verify           | 12     | 0      | 1       | 0     | 13    | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 156    | 0      | 13      | 1     | 169   | ❗     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • Eugie Limpin added 283 commits

    added 283 commits

    Compare with previous version

  • Eugie Limpin added 1 commit

    added 1 commit

    • 384ffffe - Use GlCollapsibleListbox in ci_environments_dropdown

    Compare with previous version

  • Eugie Limpin changed the description

    changed the description

  • Eugie Limpin marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • Eugie Limpin changed the description

    changed the description

  • Eugie Limpin changed the description

    changed the description

  • Eugie Limpin
  • Eugie Limpin changed the description

    changed the description

  • 🤖 GitLab Bot 🤖 changed milestone to %15.8

    changed milestone to %15.8

  • Eugie Limpin changed the description

    changed the description

  • Eugie Limpin requested review from @bsandlin

    requested review from @bsandlin

  • added UX label

  • Please wait for Reviewer Roulette to suggest a designer for UX review, and then assign them as Reviewer. This helps evenly distribute reviews across UX.

    This message was generated automatically. You're welcome to improve it.

    • Author Maintainer

      Hi @beckalippert :wave_tone2: Could you review this MR for UX, please?

      I'm asking for a review since the migration does not retain 100% of the behavior and looks of the current implementation. This is mainly because of missing features for GlCollapsibleListbox component that we are migrating towards.

      The differences between the current and new implementations are detailed in the MR description above. I hope it helps with the review :bow_tone1:

    • LGTM, but I just want to make sure we want to continue using the term wildcard here, @phillipwells :

      image

      I would expect something like Create new: zzz here instead, but maybe "wildcard" is standard terminology? cc @katiemacoy

      Edited by Becka Lippert
    • @beckalippert I agree, I think Create new: is clearer about the action the user is taking here. If it's possible to make this change while we're in here, @eugielimpin, I say go for it :slight_smile:

      cc @katiemacoy

    • @beckalippert @phillipwells When you're in a merge request and you try to type a label that doesn't exist, here is the behavior:

      image

      The word "create" means the same as "new" so you don't need to have both words, usually. (You can't create something that's old, if that makes sense.)

      Just wondering how you're using "wildcard" in this case--are you saying they're creating a search string? So if they create "foo" then it will search for *foo*?

      Anyway, this is just food for thought. I'm on vacation and happened to see this MR, so just adding some things to think about. Thanks!

    • Author Maintainer

      From the doc page for this feature

      You can use specific matching to select a particular environment. You can also use wildcard matching () to select a particular environment group, like Review Apps (review/).

      So the user can create either a scope that matches a specific environment or a "wildcard" scope that can match multiple environments.

      Perhaps "Create wildcard:" is a bit misleading :thinking:

    • @beckalippert @eugielimpin @phillipwells @sselhorn - Some expert feedback on why I recommend we keep the Create wildcard UI copy here:

      Create new is not the same as creating a wildcard in this context, because the user can create either a new environment scope OR a new wildcard for a series of environments.

      We also use wildcards in context with protecting branches, environments, artefacts, and deploy freezes.

      From the docs:

      If the environment scope is review/*, then jobs with environment names starting with review/ would have that variable available.

      The most specific spec takes precedence over the other wildcard matching. In this case, the review/feature-1 spec takes precedence over review/* and * specs.

      Here’s where we worked to improve how environment wildcards are created #5313 (closed) . We also added a link to the docs next to the input label to help users find more info about these wildcards #231529 (closed)

      I recommend that a follow-up issue is created to discuss further, since this interaction and language appears in other product areas of CD.

      Hope that helps! Let me know :smile: cc @emilybauman

    • @beckalippert Just to be clear, UX is fine with all the visual / UX changes (dropdown is not full width, search does not clear on close / select...) and we don't want to keep any of those?

      If that's not the case, we should create a follow-up to at least re-visit these if / when GitLab UI is updated with more functionality. (In its current state, the listbox is still very much WIP and lacks a few features)

    • I assume the Foundations team is aware of the current listbox component limitations and it will continue to be improved iteratively; can you comment @gitlab-com/gitlab-ux/ux-foundations?

    • @beckalippert @justin_ho

      If that's not the case, we should create a follow-up to at least re-visit these if / when GitLab UI is updated with more functionality. (In its current state, the listbox is still very much WIP and lacks a few features)

      If you find any issues with the new listbox feel free to create issues for them.

      I assume the Foundations team is aware of the current listbox component limitations and it will continue to be improved iteratively

      Of course

    • Please register or sign in to reply
  • Eugie Limpin requested review from @beckalippert

    requested review from @beckalippert

  • Becka Lippert approved this merge request

    approved this merge request

  • :wave: @beckalippert, 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:

  • added Technical Writing UI text + 1 deleted label

  • Briley Sandlin approved this merge request

    approved this merge request

  • Briley Sandlin requested review from @justin_ho and removed review request for @bsandlin

    requested review from @justin_ho and removed review request for @bsandlin

  • Justin Ho Tuan Duong removed review request for @justin_ho

    removed review request for @justin_ho

  • Eugie Limpin added 3635 commits

    added 3635 commits

    Compare with previous version

  • Eugie Limpin requested review from @justin_ho

    requested review from @justin_ho

  • Justin Ho Tuan Duong approved this merge request

    approved this merge request

  • Justin Ho Tuan Duong resolved all threads

    resolved all threads

  • Justin Ho Tuan Duong enabled an automatic merge when the pipeline for c00df4bc succeeds

    enabled an automatic merge when the pipeline for c00df4bc succeeds

  • mentioned in commit 5a7ec0fe

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue #384285 (closed)

  • Please register or sign in to reply
    Loading