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

Files busy signal #3973

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Files busy signal #3973

merged 7 commits into from
Nov 27, 2024

Conversation

ahmed-mgd
Copy link
Contributor

Fixes #1189

@@ -10,6 +10,7 @@ const EVENTNAME = {
};

const CONTENTID = '#directory-contents';
const BUSYID = '#tloading-spinner';
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we use hyphens in other places, but we're trying to enforce snake_case Ids now.

@@ -24,6 +24,13 @@
<tbody>
</tbody>
</table>

<div class="text-center text-primary" style="display:none" id="tloading-spinner">
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of inline styles, we should be using the class d-none. inline styles are a CSP violation (or at least will be at some point) so we should avoid using them.

So I think instead of the jQuery API for hide & show, we should just edit the classList in plain javascript.

// API to add to remove from the classList
document.getElementById('directory-contents').classList.remove('d-none');

// API to add to the classList
document.getElementById('directory-contents').classList.add('d-none');


<div class="text-center text-primary" style="display:none" id="tloading-spinner">
<div class="spinner-border">
<span class="visually-hidden">Loading...</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Never seen visually-hidden before, but we tend to use sr-only.

@johrstrom
Copy link
Contributor

Not sure why tests fail, but clearly the test is moving too fast for the system to actually remove the file. Putting a sleep here tends to make the test pass for me.

--- a/apps/dashboard/test/system/remote_files_test.rb
+++ b/apps/dashboard/test/system/remote_files_test.rb
@@ -279,6 +279,9 @@ class RemoteFilesTest < ApplicationSystemTestCase
         find('#delete-btn').click
         find('button.swal2-confirm').click
 
+        # sleep for the file to actually be removed.
+        sleep 1
+
         # verify app dir deleted according to UI
         assert_no_selector 'tbody a', exact_text: 'app', wait: 10
         assert_no_selector 'tbody a', exact_text: 'foo.txt', wait: 10

@@ -0,0 +1,5 @@
<div class="text-center text-primary d-none p-500" id="tloading_spinner">
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the p-500 class here? I can't seem to find a definition for it./

Copy link
Contributor Author

@ahmed-mgd ahmed-mgd Nov 21, 2024

Choose a reason for hiding this comment

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

Whoops I was messing around with the styling and forgot to delete that.

@johrstrom johrstrom merged commit 676275c into master Nov 27, 2024
25 checks passed
@johrstrom johrstrom deleted the files-busy-signal branch November 27, 2024 15:17
@johrstrom
Copy link
Contributor

Thanks!

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.

Files app needs to display a busy signal when navigating to a new location
3 participants