Skip to content
Snippets Groups Projects

Make dark mode label text and remove label buttons the same color

All threads resolved!

What does this MR do and why?

Currently in dark mode, non-scoped labels that have a remove button will have different colored label text and remove buttons. This MR makes them both the same color.

CAVEAT: there are still problems in #428092 (closed) with accessible color contrast and scoped labels that are NOT ADDRESSED in this MR.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After
Screenshot_2023-10-12_at_17.10.13 Screenshot_2023-10-12_at_17.09.57

How to set up and validate locally

  1. start your local gdk
  2. if your local gdk user is not already using dark mode, go to http://gdk.test:3000/-/profile/preferences and turn on dark mode
  3. navigate to an issue with labels. You will observe that their text color and remove button color do not match
  4. in the terminal, navigate to the gitlab directory in the gdk and check out this branch
  5. when the changes in this branch are available in the dom, you will see that the labels on the issue have text and remove buttons that are the same color

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #428092 (closed)

Edited by Chad Lavimoniere

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
  • Chad Lavimoniere mentioned in merge request !134020 (closed)

    mentioned in merge request !134020 (closed)

  • A deleted user added frontend label

    added frontend label

  • Contributor
    1 Warning
    :warning: You've made some app changes, but didn't add any tests.
    That's OK as long as you're refactoring existing code,
    but please consider adding any of the maintenancepipelines, maintenancerefactor, maintenanceworkflow, documentation, QA labels.

    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 thomasrandolph current availability (UTC-6, 2 hours behind @clavimoniere) jivanvl current availability (UTC-6, 2 hours behind @clavimoniere)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    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

  • Chad Lavimoniere requested review from @thutterer and @jivanvl

    requested review from @thutterer and @jivanvl

  • added 1 commit

    Compare with previous version

  • (force pushed a git commit --amend to fix Danger complaints about commit subject)

  • Chad Lavimoniere 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

  • Contributor

    Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 9580a2c7 and 4576b120

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 4.13 MB 4.13 MB - 0.0 %
    mainChunk 3.08 MB 3.08 MB - 0.0 %

    Note: We do not have exact data for 9580a2c7. So we have used data from: 11b108a8.
    The target commit was too new, so we used the latest commit from master we have info on.
    It might help to rerun the bundle-size-review job
    This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.

    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: :white_check_mark: test report for 4576b120

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Create           | 40     | 0      | 6       | 0     | 46    | ✅     |
    | Data Stores      | 18     | 0      | 4       | 0     | 22    | ✅     |
    | Verify           | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Manage           | 13     | 0      | 1       | 0     | 14    | ✅     |
    | Govern           | 35     | 0      | 0       | 0     | 35    | ✅     |
    | Plan             | 55     | 0      | 0       | 0     | 55    | ✅     |
    | Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 173    | 0      | 13      | 0     | 186   | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :x: test report for 4576b120

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Create           | 523    | 0      | 115     | 16    | 638   | ❗     |
    | Plan             | 241    | 0      | 10      | 0     | 251   | ✅     |
    | Release          | 15     | 0      | 3       | 0     | 18    | ✅     |
    | Verify           | 150    | 0      | 15      | 3     | 165   | ❗     |
    | Manage           | 152    | 1      | 16      | 5     | 169   | ❌     |
    | Govern           | 187    | 0      | 3       | 3     | 190   | ❗     |
    | ModelOps         | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Package          | 226    | 0      | 15      | 0     | 241   | ✅     |
    | Systems          | 8      | 0      | 0       | 0     | 8     | ✅     |
    | GitLab Metrics   | 2      | 0      | 1       | 1     | 3     | ❗     |
    | Analytics        | 7      | 0      | 0       | 0     | 7     | ✅     |
    | Fulfillment      | 8      | 0      | 69      | 0     | 77    | ✅     |
    | Monitor          | 28     | 0      | 7       | 0     | 35    | ✅     |
    | Configure        | 1      | 0      | 9       | 0     | 10    | ✅     |
    | Data Stores      | 101    | 0      | 15      | 0     | 116   | ✅     |
    | Growth           | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Secure           | 6      | 0      | 3       | 0     | 9     | ✅     |
    | Framework sanity | 0      | 0      | 3       | 0     | 3     | ➖     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 1655   | 1      | 296     | 28    | 1952  | ❌     |
    +------------------+--------+--------+---------+-------+-------+--------+

    e2e-review-qa: :white_check_mark: test report for 4576b120

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Create           | 8      | 0      | 1       | 0     | 9     | ✅     |
    | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Manage           | 1      | 0      | 0       | 0     | 1     | ✅     |
    | Plan             | 3      | 0      | 1       | 0     | 4     | ✅     |
    | Data Stores      | 1      | 0      | 1       | 0     | 2     | ✅     |
    | Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 19     | 0      | 5       | 0     | 24    | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • Chad Lavimoniere changed title from In dark mode, label text and remove label buttons should now be the same color. to Make dark mode label text and remove label buttons the same color

    changed title from In dark mode, label text and remove label buttons should now be the same color. to Make dark mode label text and remove label buttons the same color

  • mentioned in issue #428092 (closed)

  • added UX label

  • requested review from @seggenberger

  • Hey @clavimoniere :wave:

    This LGTM, I've left you some code simplifications, but from UX approved :sparkles:

  • Sascha Eggenberger approved this merge request

    approved this merge request

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

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

    For more info, please refer to the following links:

  • Sascha Eggenberger removed review request for @seggenberger

    removed review request for @seggenberger

  • added 1 commit

    • e7f9b85a - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Chad Lavimoniere resolved all threads

    resolved all threads

  • added 1 commit

    • 4576b120 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Contributor

    @clavimoniere Some end-to-end (E2E) tests should run based on the stage label.

    Please start the trigger-omnibus-and-follow-up-e2e job in the qa stage and ensure tests in the follow-up-e2e:package-and-test-ee pipeline pass before this MR is merged. (E2E tests are computationally intensive and don't run automatically for every push/rebase, so we ask you to run this job manually at least once.)

    To run all E2E tests, apply the pipeline:run-all-e2e label and run a new pipeline.

    E2E test jobs are allowed to fail due to flakiness. See current failures at the latest pipeline triage issue.

    Once done, apply the :white_check_mark: emoji on this comment.

    Team members only: for any questions or help, reach out on the internal #quality Slack channel.

  • Jose Ivan Vargas changed milestone to %16.5

    changed milestone to %16.5

  • Jose Ivan Vargas approved this merge request

    approved this merge request

  • Jose Ivan Vargas removed review request for @jivanvl

    removed review request for @jivanvl

  • Thomas Hutterer removed review request for @thutterer

    removed review request for @thutterer

  • Chad Lavimoniere mentioned in merge request !134027 (merged)

    mentioned in merge request !134027 (merged)

  • Chad Lavimoniere requested review from @pslaughter

    requested review from @pslaughter

  • Chad Lavimoniere resolved all threads

    resolved all threads

  • Looking at this now! :eyes:

  • Looks like @jivanvl has already :thumbsup:'d. I also tested locally and everything looks good to me. Looks like we just need to merge.

    Starting merge...

  • Paul Slaughter approved this merge request

    approved this merge request

  • Paul Slaughter unapproved this merge request

    unapproved this merge request

  • Paul Slaughter enabled an automatic merge when the pipeline for 9a1c2fc0 succeeds

    enabled an automatic merge when the pipeline for 9a1c2fc0 succeeds

  • merged

  • Paul Slaughter mentioned in commit 58712307

    mentioned in commit 58712307

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading