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

Rework app info to use components #5170

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Rework app info to use components #5170

wants to merge 10 commits into from

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Jan 8, 2025

WHY are these changes introduced?

  1. Replaces messy string generation code with orderly use of UI-kit.
  2. Hides the web section for extension-only apps.

WHAT is this pull request doing?

Instead of the mess of string generation, we reorganize the code to generate headers and data tables, then do the actual rendering in UI kit.

To make this work nicely, I added a new TabularData concept to UI kit, which does seem useful in general. It even has a parameter to dim the leftmost column, which I think creates a pleasant effect.

App with many extensions, before:

Screenshot 2025-01-08 at 13 31 56

And after:
Screenshot 2025-01-08 at 13 30 33

Extension-only app, before:
Screenshot 2025-01-08 at 13 44 12

And after:
Screenshot 2025-01-08 at 13 42 47

App with many errors, before:
Screenshot 2025-01-08 at 15 49 27

And after:
Screenshot 2025-01-08 at 15 50 10

How to test your changes?

Run shopify app info on various apps.

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

Checklist

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

@amcaplan amcaplan force-pushed the app-info-rework branch 4 times, most recently from 76e766a to e4cc408 Compare January 9, 2025 10:50
1. Add a default relative path (otherwise relativePath returns a blank
   string)
2. Move roles to a separate line item
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75% (-0.14% 🔻)
8849/11799
🟡 Branches
70.11% (-0.2% 🔻)
4322/6165
🟡 Functions
74.94% (-0.12% 🔻)
2318/3093
🟡 Lines
75.55% (-0.14% 🔻)
8366/11073
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / TabularData.tsx
12.5% 0% 0% 12.5%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / info.ts
81.44% (-6.74% 🔻)
60.94% (-2.52% 🔻)
90.32%
83.15% (-7.85% 🔻)
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%
🟢
... / Alert.tsx
100%
92% (-3.24% 🔻)
100% 100%
🟢
... / FatalError.tsx
100%
86.11% (-1.39% 🔻)
100% 100%

Test suite run success

1998 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 413f784

We removed the upgrade reminders in #4310
but we missed deleting the tests!
@amcaplan amcaplan force-pushed the app-info-rework branch 2 times, most recently from 3bf58a5 to 2d18f38 Compare January 9, 2025 14:27
@amcaplan amcaplan marked this pull request as ready for review January 9, 2025 15:32
@amcaplan amcaplan requested a review from a team as a code owner January 9, 2025 15:32

This comment has been minimized.

@amcaplan amcaplan requested a review from a team as a code owner January 9, 2025 15:51
Copy link
Contributor

@graygilmore graygilmore left a comment

Choose a reason for hiding this comment

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

I won't speak to the introduction of a new component but this is such a massive improvement for apps – thank you!

this.tableSection(
'Current app configuration',
[
['Configuration file', {filePath: basename(this.app.configuration.path) || configurationFileNames.app}],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just flagging that the filePath token gets rendered pretty differently depending on your terminal colors. I don't mind that it's different but I wanted to point it out!

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is very surprising! It's supposed to be italicized, I wonder what your settings are such that colors are inverted instead.

Copy link
Contributor

github-actions bot commented Jan 9, 2025

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

packages/cli-kit/dist/private/node/ui/components/TabularData.d.ts
import { InlineToken } from './TokenizedText.js';
import { FunctionComponent } from 'react';
export interface TabularDataProps {
    tabularData: InlineToken[][];
    firstColumnSubdued?: boolean;
}
declare const TabularData: FunctionComponent<TabularDataProps>;
export { TabularData };

Existing type declarations

packages/cli-kit/dist/private/node/ui/components/Alert.d.ts
@@ -1,9 +1,10 @@
 import { BannerType } from './Banner.js';
 import { BoldToken, InlineToken, LinkToken, TokenItem } from './TokenizedText.js';
+import { TabularDataProps } from './TabularData.js';
 import { FunctionComponent } from 'react';
 export interface CustomSection {
     title?: string;
-    body: TokenItem;
+    body: TabularDataProps | TokenItem;
 }
 export interface AlertProps {
     type: Exclude<BannerType, 'external_error'>;

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.

2 participants