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

GoldenRetriever + hideUploadButton=true #5350

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Conversation

lakesare
Copy link
Collaborator

Fixes: #5142

GoldenRetriever + { hideUploadButton: true }

Before

image

After

image

Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/status-bar/lib/StatusBarUI.js b/packages/@uppy/status-bar/lib/StatusBarUI.js
index 66e3513..e518609 100644
--- a/packages/@uppy/status-bar/lib/StatusBarUI.js
+++ b/packages/@uppy/status-bar/lib/StatusBarUI.js
@@ -95,21 +95,7 @@ export default function StatusBarUI(_ref) {
         return false;
     }
   }
-  function getIsHidden() {
-    if (recoveredState) {
-      return false;
-    }
-    switch (uploadState) {
-      case STATE_WAITING:
-        return hideUploadButton || newFiles === 0;
-      case STATE_COMPLETE:
-        return hideAfterFinish;
-      default:
-        return false;
-    }
-  }
   const progressValue = getProgressValue();
-  const isHidden = getIsHidden();
   const width = progressValue != null ? progressValue : 100;
   const showUploadBtn = !error && newFiles && !isUploadInProgress && !isAllPaused && allowNewUpload
     && !hideUploadButton;
@@ -123,6 +109,48 @@ export default function StatusBarUI(_ref) {
   const statusBarClassNames = classNames("uppy-StatusBar", `is-${uploadState}`, {
     "has-ghosts": isSomeGhost,
   });
+  const progressBarStateEl = (() => {
+    switch (uploadState) {
+      case STATE_PREPROCESSING:
+      case STATE_POSTPROCESSING:
+        return h(ProgressBarProcessing, {
+          progress: calculateProcessingProgress(files),
+        });
+      case STATE_COMPLETE:
+        return h(ProgressBarComplete, {
+          i18n: i18n,
+        });
+      case STATE_ERROR:
+        return h(ProgressBarError, {
+          error: error,
+          i18n: i18n,
+          numUploads: numUploads,
+          complete: complete,
+        });
+      case STATE_UPLOADING:
+        return h(ProgressBarUploading, {
+          i18n: i18n,
+          supportsUploadProgress: supportsUploadProgress,
+          totalProgress: totalProgress,
+          showProgressDetails: showProgressDetails,
+          isUploadStarted: isUploadStarted,
+          isAllComplete: isAllComplete,
+          isAllPaused: isAllPaused,
+          newFiles: newFiles,
+          numUploads: numUploads,
+          complete: complete,
+          totalUploadedSize: totalUploadedSize,
+          totalSize: totalSize,
+          totalETA: totalETA,
+          startUpload: startUpload,
+        });
+      default:
+        return null;
+    }
+  })();
+  const atLeastOneAction = showUploadBtn || showRetryBtn || showPauseResumeBtn || showCancelBtn || showDoneBtn;
+  const thereIsNothingInside = !atLeastOneAction && !progressBarStateEl;
+  const isHidden = thereIsNothingInside || uploadState === STATE_COMPLETE && hideAfterFinish;
   return h(
     "div",
     {
@@ -141,51 +169,13 @@ export default function StatusBarUI(_ref) {
       "aria-valuemax": 100,
       "aria-valuenow": progressValue,
     }),
-    (() => {
-      switch (uploadState) {
-        case STATE_PREPROCESSING:
-        case STATE_POSTPROCESSING:
-          return h(ProgressBarProcessing, {
-            progress: calculateProcessingProgress(files),
-          });
-        case STATE_COMPLETE:
-          return h(ProgressBarComplete, {
-            i18n: i18n,
-          });
-        case STATE_ERROR:
-          return h(ProgressBarError, {
-            error: error,
-            i18n: i18n,
-            numUploads: numUploads,
-            complete: complete,
-          });
-        case STATE_UPLOADING:
-          return h(ProgressBarUploading, {
-            i18n: i18n,
-            supportsUploadProgress: supportsUploadProgress,
-            totalProgress: totalProgress,
-            showProgressDetails: showProgressDetails,
-            isUploadStarted: isUploadStarted,
-            isAllComplete: isAllComplete,
-            isAllPaused: isAllPaused,
-            newFiles: newFiles,
-            numUploads: numUploads,
-            complete: complete,
-            totalUploadedSize: totalUploadedSize,
-            totalSize: totalSize,
-            totalETA: totalETA,
-            startUpload: startUpload,
-          });
-        default:
-          return null;
-      }
-    })(),
+    progressBarStateEl,
     h(
       "div",
       {
         className: "uppy-StatusBar-actions",
       },
-      recoveredState || showUploadBtn
+      showUploadBtn
         ? h(UploadBtn, {
           newFiles: newFiles,
           isUploadStarted: isUploadStarted,

@@ -234,7 +240,7 @@ export default function StatusBarUI<M extends Meta, B extends Body>({
{progressBarStateEl}

<div className="uppy-StatusBar-actions">
{recoveredState || showUploadBtn ?
{showUploadBtn ?
Copy link
Collaborator Author

@lakesare lakesare Jul 16, 2024

Choose a reason for hiding this comment

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

This is the main change, everything else is here in order to make the footer collapse when nothing's inside

image

@lakesare lakesare marked this pull request as ready for review July 16, 2024 17:34
@lakesare lakesare requested review from aduh95 and Murderlon and removed request for aduh95 July 16, 2024 17:34
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Nice fix 👍

return null
}
})()}
{progressBarStateEl}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can undo this change. Makes the PR more clear and it's not necessarily an improvement.

Copy link
Collaborator Author

@lakesare lakesare Jul 17, 2024

Choose a reason for hiding this comment

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

It's not a prettification, we take progressBarStateEl up because we need that value in order to determine whether we should collapse the footer.

In normal html, this footer would be automatically collapsed when nothing's inside. In our case, we collapse it manually via the height property (controlled by aria-hidden), because we need animations.
My point with this change is to make this aria-hidden derived.
Nothing is inside (each child determines via some fancy logic whether it needs to be here)? Collapse the footer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you like we can revert this change and write

const thereIsNothingInside = !atLeastOneAction && uploadState === STATE_WAITING

(logically equivalent, but aria-hidden stops being derived from the presence of children).

I like this solution too, was on the brim here.

@lakesare lakesare merged commit 160cf16 into main Jul 22, 2024
17 checks passed
@lakesare lakesare deleted the lakesare/hideUploadButton branch July 22, 2024 23:44
@github-actions github-actions bot mentioned this pull request Jul 30, 2024
github-actions bot added a commit that referenced this pull request Jul 30, 2024
| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/audio          |   2.0.1 | @uppy/status-bar     |   4.0.1 |
| @uppy/aws-s3         |   4.0.2 | @uppy/store-default  |   4.1.0 |
| @uppy/compressor     |   2.0.1 | @uppy/transloadit    |   4.0.1 |
| @uppy/core           |   4.1.0 | @uppy/utils          |   6.0.1 |
| @uppy/dashboard      |   4.0.2 | @uppy/webcam         |   4.0.1 |
| @uppy/remote-sources |   2.1.0 | uppy                 |   4.1.0 |

- @uppy/remote-sources: fix options type (Merlijn Vos / #5364)
- @uppy/transloadit: do not mark `opts` as mandatory (Antoine du Hamel / #5375)
- @uppy/compressor: mark `quality` as optional (Antoine du Hamel / #5374)
- @uppy/aws-s3: improve error when `endpoint` is not provided (Antoine du Hamel / #5361)
- @uppy/core,@uppy/store-default: export `Store` type (Merlijn Vos / #5373)
- @uppy/dashboard: make `toggleAddFilesPanel` args consistent (Evgenia Karunus / #5365)
- @uppy/dashboard: Dashboard - convert some files to typescript (Evgenia Karunus / #5359)
- @uppy/audio,@uppy/webcam: Don't use `<h1>` in Uppy markup (Evgenia Karunus / #5355)
- @uppy/status-bar: GoldenRetriever + `hideUploadButton=true` (Evgenia Karunus / #5350)
- meta: Bump docker/build-push-action from 6.3.0 to 6.4.1 (dependabot[bot] / #5360)
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.

Dashboard - status bar upload button visible after restoring files, despite hideUploadButton being true.
2 participants