Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace rainbow bar with moving hill for no-color mode #5262

Merged
merged 5 commits into from
Jan 26, 2025

Conversation

amcaplan
Copy link
Contributor

WHY are these changes introduced?

Fixes the poor display of the loading bar in no-color terminals.

image

WHAT is this pull request doing?

Screen.Recording.2025-01-23.at.17.45.20.mov

How to test your changes?

$ p shopify kitchen-sink async

or any other command which uses a rainbow bar (e.g. creating a new app)

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact
  • Impact is unknown, a followup PR will includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@amcaplan amcaplan requested a review from a team as a code owner January 23, 2025 15:47

This comment has been minimized.

@amcaplan amcaplan force-pushed the no-color-loader-bar branch from e6717a9 to a2e5f06 Compare January 23, 2025 16:14
@amcaplan amcaplan requested a review from a team as a code owner January 23, 2025 16:14
@amcaplan amcaplan force-pushed the no-color-loader-bar branch from a2e5f06 to 913eb0d Compare January 23, 2025 18:00
@amcaplan amcaplan force-pushed the no-color-loader-bar branch from 913eb0d to 4aee075 Compare January 23, 2025 19:10
Copy link
Contributor

github-actions bot commented Jan 23, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.34% (+0.02% 🔼)
8899/11812
🟡 Branches
70.57% (+0.03% 🔼)
4328/6133
🟡 Functions
75.11% (+0.02% 🔼)
2339/3114
🟡 Lines
75.88% (+0.03% 🔼)
8411/11085
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% 90.48% 98.61%
🟢
... / TextAnimation.tsx
100%
50% (-50% 🔻)
100% 100%

Test suite run success

2003 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 549c884

@amcaplan amcaplan force-pushed the no-color-loader-bar branch 2 times, most recently from 19b7685 to 13716fb Compare January 23, 2025 20:32
@@ -25,6 +27,7 @@ interface TasksProps<TContext> {
silent?: boolean
onComplete?: (ctx: TContext) => void
abortSignal?: AbortSignal
noColor?: boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only introduced this to inject true in tests because mocking output.js (where we get shouldDisplayColors) led to very weird results. It's a private interface, so I don't think this should matter all that much.

const loadingBar = new Array(twoThirds).fill(loadingBarChar).join('')
let loadingBar = new Array(twoThirds).fill(loadingBarChar).join('')
if (noColor ?? !shouldDisplayColors()) {
loadingBar = hillString.repeat(Math.ceil(twoThirds / hillString.length))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rounding up here so we have a bit of extra insurance at the end (which TextAnimation will truncate), rather than overtruncating immediately.

@amcaplan amcaplan force-pushed the no-color-loader-bar branch from 13716fb to 549c884 Compare January 23, 2025 20:41
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/ui/components/Tasks.d.ts
@@ -13,6 +13,7 @@ interface TasksProps<TContext> {
     silent?: boolean;
     onComplete?: (ctx: TContext) => void;
     abortSignal?: AbortSignal;
+    noColor?: boolean;
 }
-declare function Tasks<TContext>({ tasks, silent, onComplete, abortSignal, }: React.PropsWithChildren<TasksProps<TContext>>): JSX.Element | null;
+declare function Tasks<TContext>({ tasks, silent, onComplete, abortSignal, noColor, }: React.PropsWithChildren<TasksProps<TContext>>): JSX.Element | null;
 export { Tasks };
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/TextAnimation.d.ts
@@ -1,9 +1,10 @@
 import React from 'react';
 interface TextAnimationProps {
     text: string;
+    maxWidth?: number;
 }
 /**
  *  applies a rainbow animation to text.
  */
-declare const TextAnimation: React.MemoExoticComponent<({ text }: TextAnimationProps) => JSX.Element>;
+declare const TextAnimation: React.MemoExoticComponent<({ text, maxWidth }: TextAnimationProps) => JSX.Element>;
 export { TextAnimation };
\ No newline at end of file

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @amcaplan! Very neat accessibility enhancement! LGTM and works as expected as well:

tophat-2

@amcaplan amcaplan added this pull request to the merge queue Jan 26, 2025
Merged via the queue into main with commit 33f6fb8 Jan 26, 2025
27 checks passed
@amcaplan amcaplan deleted the no-color-loader-bar branch January 26, 2025 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants