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

Add-a-spinner-at-least-until-some-messages-are-shown #1126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

David-Muhasa
Copy link
Member

As part of, or after Bootstrap 5: Cypht: Add a spinner at least until some messages are shown
In lists
For a message
Search
etc.

Copy link
Member

@Shadow243 Shadow243 left a comment

Choose a reason for hiding this comment

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

Please make sure all tests pass successfully

Copy link
Member

@Shadow243 Shadow243 left a comment

Choose a reason for hiding this comment

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

I like the idea there is just one problem when we are on the compose
page, cypht is making ajax requests on the hook: ajax_imap_message_content and this is preventing someone from continuing to write until that this request ends, several times.

Screenshot 2024-07-25 at 18 00 18

Try to find a solution to this.

@Shadow243
Copy link
Member

@Shadow243
Copy link
Member

Shadow243 commented Aug 3, 2024

You changed loading_icon to

<div class="loading_bar"></div>\n
   │ +            <div class="loading_spinner text-center">\n
   │ +                <div class="spinner-border" style="width: 3rem; height: 3rem;" role="status">\n
   │ +                    <span class="visually-hidden">Loading...</span>\n
   │ +                </div>\n
   │ +            </div>'

Test:
✘ Loading icon
at:
tests/phpunit/modules/core/output_modules.php:459

@David-Muhasa
Copy link
Member Author

I changed loading_icon to loading_bar

You changed loading_icon to

<div class="loading_bar"></div>\n
   │ +            <div class="loading_spinner text-center">\n
   │ +                <div class="spinner-border" style="width: 3rem; height: 3rem;" role="status">\n
   │ +                    <span class="visually-hidden">Loading...</span>\n
   │ +                </div>\n
   │ +            </div>'

Test: ✘ Loading icon at: tests/phpunit/modules/core/output_modules.php:459

I changed loading icon to loading bar to be very specific

@Shadow243
Copy link
Member

I changed loading_icon to loading_bar

You changed loading_icon to

<div class="loading_bar"></div>\n
   │ +            <div class="loading_spinner text-center">\n
   │ +                <div class="spinner-border" style="width: 3rem; height: 3rem;" role="status">\n
   │ +                    <span class="visually-hidden">Loading...</span>\n
   │ +                </div>\n
   │ +            </div>'

Test: ✘ Loading icon at: tests/phpunit/modules/core/output_modules.php:459

I changed loading icon to loading bar to be very specific

Okay, you also need to go change this in the unit tests at this line: tests/phpunit/modules/core/output_modules.php:459

@@ -650,7 +650,13 @@ class Hm_Output_loading_icon extends Hm_Output_Module {
* Sort of ugly loading icon animated with js/css
*/
protected function output() {
return '<div class="loading_icon"></div>';
Copy link
Member

Choose a reason for hiding this comment

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

You renamed the class here: class="loading_bar" but it was refering to the module: Hm_Output_loading_icon you should keep it like that.

@Shadow243
Copy link
Member

I'm trying to think of an example from which we can get an idea: nprogress

Can you do something like that ?

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