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

@uppy/dashboard: stylistic changes for plugin options #5384

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

lakesare
Copy link
Contributor

@lakesare lakesare commented Aug 2, 2024

This PR lays the ground for #5377.

This PR:

  • adds trigger to default options
  • groups modal and inline options (decorative change that will ease the diffs later on)
  • adds all options that exist to default options - onDragLeave, onDragOver, onDrop, plugins (here we're gradually getting rid of undefined as a valid value for the option)

Only merge after #5367.
Before merging, please change base back to the main branch.

@lakesare lakesare marked this pull request as ready for review August 2, 2024 11:03
Copy link
Contributor

github-actions bot commented Aug 2, 2024

Diff output files
diff --git a/packages/@uppy/dashboard/lib/Dashboard.js b/packages/@uppy/dashboard/lib/Dashboard.js
index 929c8e2..cc7e123 100644
--- a/packages/@uppy/dashboard/lib/Dashboard.js
+++ b/packages/@uppy/dashboard/lib/Dashboard.js
@@ -37,9 +37,6 @@ function createPromise() {
 const defaultOptions = {
   target: "body",
   metaFields: [],
-  inline: false,
-  width: 750,
-  height: 550,
   thumbnailWidth: 280,
   thumbnailType: "image/jpeg",
   waitForThumbnailsBeforeUpload: false,
@@ -52,19 +49,14 @@ const defaultOptions = {
   hidePauseResumeButton: false,
   hideProgressAfterFinish: false,
   note: null,
-  closeModalOnClickOutside: false,
-  closeAfterFinish: false,
   singleFileFullScreen: true,
   disableStatusBar: false,
   disableInformer: false,
   disableThumbnailGenerator: false,
-  disablePageScrollWhenModalOpen: true,
-  animateOpenClose: true,
   fileManagerSelectionType: "files",
   proudlyDisplayPoweredByUppy: true,
   showSelectedFiles: true,
   showRemoveButtonAfterComplete: false,
-  browserBackButtonClose: false,
   showNativePhotoCameraButton: false,
   showNativeVideoCameraButton: false,
   theme: "light",
@@ -72,8 +64,21 @@ const defaultOptions = {
   disabled: false,
   disableLocalFiles: false,
   nativeCameraFacingMode: "",
+  onDragLeave: () => {},
+  onDragOver: () => {},
+  onDrop: () => {},
+  plugins: [],
   doneButtonHandler: undefined,
   onRequestCloseModal: null,
+  inline: false,
+  animateOpenClose: true,
+  browserBackButtonClose: false,
+  closeAfterFinish: false,
+  closeModalOnClickOutside: false,
+  disablePageScrollWhenModalOpen: true,
+  trigger: null,
+  width: 750,
+  height: 550,
 };
 var _disabledNodes = _classPrivateFieldLooseKey("disabledNodes");
 var _generateLargeThumbnailIfSingleFile = _classPrivateFieldLooseKey("generateLargeThumbnailIfSingleFile");
@@ -94,7 +99,7 @@ var _getThumbnailGeneratorId = _classPrivateFieldLooseKey("getThumbnailGenerator
 var _getInformerId = _classPrivateFieldLooseKey("getInformerId");
 export default class Dashboard extends UIPlugin {
   constructor(uppy, _opts) {
-    var _opts$autoOpen, _this$opts4, _this$opts4$onRequest;
+    var _opts$autoOpen, _this$opts, _this$opts$onRequestC;
     const autoOpen = (_opts$autoOpen = _opts == null ? void 0 : _opts.autoOpen) != null ? _opts$autoOpen : null;
     super(uppy, {
       ...defaultOptions,
@@ -502,7 +507,6 @@ export default class Dashboard extends UIPlugin {
       }
     };
     this.handleDragOver = event => {
-      var _this$opts$onDragOver, _this$opts;
       event.preventDefault();
       event.stopPropagation();
       const canSomePluginHandleRootDrop = () => {
@@ -534,21 +538,17 @@ export default class Dashboard extends UIPlugin {
       this.setPluginState({
         isDraggingOver: true,
       });
-      (_this$opts$onDragOver = (_this$opts = this.opts).onDragOver) == null
-        || _this$opts$onDragOver.call(_this$opts, event);
+      this.opts.onDragOver(event);
     };
     this.handleDragLeave = event => {
-      var _this$opts$onDragLeav, _this$opts2;
       event.preventDefault();
       event.stopPropagation();
       this.setPluginState({
         isDraggingOver: false,
       });
-      (_this$opts$onDragLeav = (_this$opts2 = this.opts).onDragLeave) == null
-        || _this$opts$onDragLeav.call(_this$opts2, event);
+      this.opts.onDragLeave(event);
     };
     this.handleDrop = async event => {
-      var _this$opts$onDrop, _this$opts3;
       event.preventDefault();
       event.stopPropagation();
       this.setPluginState({
@@ -575,7 +575,7 @@ export default class Dashboard extends UIPlugin {
         this.uppy.log("[Dashboard] Files dropped");
         this.addFiles(files);
       }
-      (_this$opts$onDrop = (_this$opts3 = this.opts).onDrop) == null || _this$opts$onDrop.call(_this$opts3, event);
+      this.opts.onDrop(event);
     };
     this.handleRequestThumbnail = file => {
       if (!this.opts.waitForThumbnailsBeforeUpload) {
@@ -922,7 +922,9 @@ export default class Dashboard extends UIPlugin {
     Object.defineProperty(this, _addSpecifiedPluginsFromOptions, {
       writable: true,
       value: () => {
-        const plugins = this.opts.plugins || [];
+        const {
+          plugins,
+        } = this.opts;
         plugins.forEach(pluginID => {
           const plugin = this.uppy.getPlugin(pluginID);
           if (plugin) {
@@ -1047,7 +1049,9 @@ export default class Dashboard extends UIPlugin {
         const thumbnail = this.uppy.getPlugin(`${this.id}:ThumbnailGenerator`);
         if (thumbnail) this.uppy.removePlugin(thumbnail);
       }
-      const plugins = this.opts.plugins || [];
+      const {
+        plugins,
+      } = this.opts;
       plugins.forEach(pluginID => {
         const plugin = this.uppy.getPlugin(pluginID);
         if (plugin) plugin.unmount();
@@ -1073,9 +1077,9 @@ export default class Dashboard extends UIPlugin {
         this.requestCloseModal();
       };
     }
-    (_this$opts4$onRequest = (_this$opts4 = this.opts).onRequestCloseModal) != null
-      ? _this$opts4$onRequest
-      : _this$opts4.onRequestCloseModal = () => this.closeModal();
+    (_this$opts$onRequestC = (_this$opts = this.opts).onRequestCloseModal) != null
+      ? _this$opts$onRequestC
+      : _this$opts.onRequestCloseModal = () => this.closeModal();
     this.i18nInit();
   }
   setOptions(opts) {

@lakesare lakesare requested a review from aduh95 August 2, 2024 11:06
Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

even though reordering the options will lead to a large diff, it will lead to better code readability which is more important imo

@@ -170,7 +170,7 @@ interface DashboardMiscOptions<M extends Meta, B extends Body>
thumbnailHeight?: number
thumbnailType?: string
thumbnailWidth?: number
trigger?: string | Element
trigger?: string | Element | null
Copy link
Contributor

Choose a reason for hiding this comment

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

for people who have a strict tsconfig and have set exactOptionalPropertyTypes: true, it's nice to also allow undefined (but this also applies to all other options, so maybe a separate pr when i think about it..)

Suggested change
trigger?: string | Element | null
trigger?: string | Element | null | undefined

Copy link
Member

Choose a reason for hiding this comment

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

imo that's what ? is for, we don't need to add undefined everywhere. Too redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

that is incorrect when exactOptionalPropertyTypes: true:

  • undefined means that the property exists on the object but has the value undefined, e.g. { prop: undefined } satisfies the type { prop: string | undefined }
  • ? means that the property might not exist at all, e.g.: {} satisfies the type { prop?: string }

And we cannot control whether or not consumers of uppy are using exactOptionalPropertyTypes: true in their tsconfig

@Murderlon Murderlon changed the title Dashboard - rework options (preliminary) @uppy/dashboard: stylistic changes for plugin options Oct 15, 2024
@Murderlon Murderlon merged commit 94b6a47 into lakesare/type-dashboard-2 Oct 15, 2024
17 checks passed
@Murderlon Murderlon deleted the lakesare/dashboard-options branch October 15, 2024 08:12
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