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

[Themes] Theme Dev - Display theme asset upload errors in overlay #4899

Closed
wants to merge 1 commit into from

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Nov 20, 2024

WHY are these changes introduced?

To improve the developer experience by providing clear visual feedback when theme files fail to upload during development.

WHAT is this pull request doing?

  • Adds an error message overlay to the browser when running theme dev and file uploads fail. This includes pre-existing errors in theme assets, as well changes that occur after the server has been initialized.
  • The overlay hot-reloads the window when a file fails upload, displaying which files failed to upload and their corresponding error messages.
  • As users upload a fix their errors, the corresponding error message is removed.
  • Users can dismiss the overlay with a close button.

Note: I believe that the hot-reload doesn't work automatically for sections that aren't being rendered / displayed on the current page. We may want to change this to a full reload or something along those lines.

How to test your changes?

  1. Run theme dev with a theme
  2. Introduce an error in a theme file (e.g., invalid Liquid syntax)
  3. Save the file and verify that:
    • An error overlay appears in the browser
    • The overlay shows the file name and error message
    • The overlay can be dismissed using the close button
    • The error persists until the file is fixed

Measuring impact

  • 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

Copy link
Contributor Author

jamesmengo commented Nov 20, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

github-actions bot commented Nov 20, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.35% (+0.01% 🔼)
8718/11570
🟡 Branches
70.67% (+0.02% 🔼)
4251/6015
🟡 Functions 75.03% 2287/3048
🟡 Lines
75.92% (+0.02% 🔼)
8246/10861
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.67% (-1.27% 🔻)
85.71% (-2.86% 🔻)
90.48% 98.59%

Test suite run success

1967 tests passing in 892 suites.

Report generated by 🧪jest coverage report action from 71baa0e

@jamesmengo jamesmengo changed the title --wip-- [skip ci] [Themes] Add error overlay for failed file uploads Nov 21, 2024
@jamesmengo jamesmengo changed the title [Themes] Add error overlay for failed file uploads [Themes] Theme Dev - Display theme asset upload errors in overlay Nov 21, 2024
@jamesmengo jamesmengo changed the base branch from main to jm/strict_push November 22, 2024 18:38
@jamesmengo jamesmengo force-pushed the jm/strict_push branch 4 times, most recently from 26ac483 to 5107f4e Compare November 27, 2024 17:39
@jamesmengo jamesmengo changed the base branch from jm/strict_push to graphite-base/4899 November 27, 2024 17:46
@jamesmengo jamesmengo changed the base branch from graphite-base/4899 to main November 29, 2024 01:54
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/public/node/themes/types.d.ts
@@ -15,6 +15,7 @@ type ThemeFSEvent = {
     type: 'add' | 'change';
     payload: {
         fileKey: Key;
+        uploadErrors: Map<Key, string[]>;
         onContent: (fn: (content: string) => void) => void;
         onSync: (fn: () => void) => void;
     };
@@ -93,6 +94,14 @@ export interface ThemeFileSystem extends VirtualFileSystem {
     applyIgnoreFilters: <T extends {
         key: string;
     }>(files: T[]) => T[];
+    /**
+     * Map of file keys to errors.
+     */
+    uploadErrors: Map<string, string[]>;
+    /**
+     * Emits an event to the event emitter.
+     */
+    emitEvent: <T extends ThemeFSEventName>(eventName: T, payload: ThemeFSEventPayload<T>) => void;
 }
 /**
  * Represents a theme on the file system.

Copy link
Contributor

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant