Added Loading in tabs

[yashrajbothra]

Fixes #3068

Short description of what this resolves:

It will add loader while the tab gets loaded

Up For Grabs demo for this PR is here:
https://www.open-emr.org/wiki/index.php/Development_Demo#Alpha_-_Up_For_Grabs_Demo

[sjpadgett]

nice tip. Now if only I can remember that decimal:)

…

On Thu, Mar 5, 2020 at 1:52 PM Tyler Wrenn ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In interface/main/tabs/templates/tabs_template.php <#3079 (comment)>: > @@ -38,6 +38,12 @@ <!-- ko foreach: tabs.tabsList --> <div class="frameDisplay" data-bind="visible:visible"> + <div class="col-sm-12" class="loader" data-bind="visible:loader"> + <div class="text-center"> + <i class="fa fa-spinner fa-pulse fa-fw" style="font-size: 140px; color: var(--gray700); padding: 20px"></i> Font size here should be using rem instead of pixels to allow responsive fonts. This is how it should be written: font-size: 8.75rem. You can get this number by multiplying pixels by 0.0625 to get the rem. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3079?email_source=notifications&email_token=AAOGDENT6WB75Y2O62PSV63RF7YHRA5CNFSM4LCEZHCKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYFRVHA#pullrequestreview-369826460>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOGDEKKJYRSVKJ4MAXJTNDRF7YHRANCNFSM4LCEZHCA> .

-- Jerry Padgett Consultant and Project Administrator for OpenEMR Brandon, Florida 33511 sjpadgett@gmail.com <sjpadgett@gmailo.com>

[sjpadgett]

Agree. Thought I mentioned spinners in issue. It may have even been loading... I replaced with New back in 5.0 or 5.0.1 which was based on user complaints whatever it was.. Do you remember what it use to be @mdsupport ?

…

On Thu, Mar 5, 2020 at 6:22 PM mdsupport ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In interface/main/tabs/templates/tabs_template.php <#3079 (comment)>: > @@ -38,6 +38,12 @@ <!-- ko foreach: tabs.tabsList --> <div class="frameDisplay" data-bind="visible:visible"> + <div class="col-sm-12" class="loader" data-bind="visible:loader"> + <div class="text-center"> + <i class="fa fa-spinner fa-pulse fa-fw" style="font-size: 140px; color: var(--gray700); padding: 20px"></i> From user's perspective spinners do not convey any information and enforce a feeling of helplessness against the *system* gradually leading to rage. If you are trying to acknowledge user's submit action, give a visible indication like changing color or change cursor and display a small normal sized text message like 'Loading ..'. If you must, it is better to use font awesome's size classes rather than any styles. 140px for 16px default would be fa-9x as a last resort. At least there won't be any inline styles. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3079?email_source=notifications&email_token=AAOGDEJ2DNKCGK6IDPOAH3TRGAXZ5A5CNFSM4LCEZHCKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYGPLNQ#discussion_r388593766>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOGDEOJNIPPLCLAUTOQKZLRGAXZ5ANCNFSM4LCEZHCA> .

-- Jerry Padgett Consultant and Project Administrator for OpenEMR Brandon, Florida 33511 sjpadgett@gmail.com <sjpadgett@gmailo.com>

[sjpadgett]

try fa-10x

[im-Amitto]

@sjpadgett Current codebase fontawesome only support till fa-5x.

[yashrajbothra]

try fa-10x

I looked into FA library in my local and wasn't able to find any fa-9x or 10x just upto 5x. 😕

[yashrajbothra]

Can we upgrade Font awesome to the latest one?

[im-Amitto]

@yashrajbothra it's not version issue, by default font-awesome comes with fa-5x though you can easily tweak the settings.
Ref: https://stackoverflow.com/a/25720069

[sjpadgett]

any fa-9x or 10x

FA skips fa-8x and fa-9x

Can we upgrade Font awesome to the latest one?

I'm not very experienced with BS4(I started at BS1 thought) so we'll let @tywrenn weigh in.
Otherwise, i'll update dependency if okay.

[yashrajbothra]

@yashrajbothra it's not version issue, by default font-awesome comes with fa-5x though you can easily tweak the settings.
Ref: https://stackoverflow.com/a/25720069

Not pretty sure we can use this or not but this contains 9x and 10x aswell
https://cdnjs.cloudflare.com/ajax/libs/font-awesome/5.12.1/js/all.min.js

[im-Amitto]

@yashrajbothra I just added that link so you can get a understanding that how it works.
You can just add fa-10x in public/assets/font-awesome/css/font-awesome.css:37 to achieve the requirment.
don't forgot to update the .min.css version because that's what we use actually

[sjpadgett]

you can easily tweak the settings.

This is not worth it and we all know my opinion using spinner here. If must leave font-size style and add appropriate fa size class. Adjust where needed.

fa-10x in public/assets/font-awesome/css/font-awesome.css:37

We don't want to do this kind of stuff.
Could just add class overrides in one of core scss.

[im-Amitto]

you can easily tweak the settings.

This is not worth it and we all know my opinion using spinner here. If must leave font-size style and add appropriate fa size class. Adjust where needed.

fa-10x in public/assets/font-awesome/css/font-awesome.css:37

We don't want to do this kind of stuff.
Could just add class overrides in one of core scss.

Note Taken.

[sjpadgett]

Note Taken.
sorry @im-Amitto my post sounded short. didn't mean it that way....

[mdsupport]

@yashrajbothra Is there any pressing need for spinners documented by community? Did anyone ask for the spinner to fill half or full screen? In fact I know we will be asked to suppress it by our users.

So without sounding like I understand the need for this PR, if you must have a huge spinner or any fa icon and built-ins are not large enough for need, preferred way to handle it would be to wrap fa-5x or whatever as
<a style="font-size:1000%"><i class="fa fa-5x fa-xyz"></i></a>
This will produce a fa-50x effect.

[sjpadgett]

I think i'll make command decision for this and say we abandon doing the spinner. We'll let Brady comment but I just know this will bite us!

I wish I could remember why I changed this the last time!!!!

[mdsupport]

Come to think of it, I wonder if project should add a fa-50x fa-stop to Site ID is missing 😉

[sjpadgett]

Come to think of it, I wonder if project should add a fa-50x fa-stop to Site ID is missing

lol, heck, why not!

[yashrajbothra]

Did anyone ask for the spinner to fill half or full screen?

No, I just copied the spinner from flow board. So its like that.

I think i'll make command decision for this and say we abandon doing the spinner. We'll let Brady comment but I just know this will bite us!

I wish I could remember why I changed this the last time!!!!

Can I Try to do what @mdsupport said rest all depends on your decision 😕

[sjpadgett]

Can I Try to do what @mdsupport said

Sure. I'll live with it for awhile:)

[im-Amitto]

Come to think of it, I wonder if project should add a fa-50x fa-stop to Site ID is missing

My 2 cents are on it 🚶‍♂️

[mdsupport]

@yashrajbothra Looks like you are really a spinner. How about leaving the body alone and setting the tab tile as :

Loading <a style="font-size:80%"><i class="fa fa-spinner"></i></a>

[yashrajbothra]

@yashrajbothra Looks like you are really a spinner. How about leaving the body alone and setting the tab tile as :

Loading <a style="font-size:80%"><i class="fa fa-spinner"></i></a>

i guess font size 80% would be very small for full frame.

If this looks good then I am in ❔

[mdsupport]

Part 1 -
For spinner - tab title would be where you are changing the js file. It is important that spinner should not increase the size of the tab which is why I started with 80%.

Part 2 -
You could leave template unchanged. I do not recall anyone complaining about it. But if you want to learn about tab mechanism, then it should display something like :

Loading Calendar ...

1. Display line can be h5/h6. It's targeted for new users so you want to make sure regular users are not distracted by font size or any moving elements.
2. Should contain translated menu string. Source could be user click or system default for initial tabs.
3. Center that string horizontally and vertically

Part 2 is purely so you get an idea how the menu mechanism works.

[bradymiller]

Considering the high activity of this PR, set up a Up For Grabs demo for this PR here:
https://www.open-emr.org/wiki/index.php/Development_Demo#Alpha_-_Up_For_Grabs_Demo
(demo uses the code from this PR and resets daily around midnight pacific time)

[bradymiller]

Regarding direct editing of public/assets/font-awesome/css/font-awesome.css , not possible since it doesn't exist in codebase since it is brought in during OpenEMR build :)

[bradymiller]

@mdsupport , here you go:

[kchapple]

Hmm, using Firefox I just see a blank screen while the frame is loading. I'll see if I can figure it out.

…

On Tue, Mar 10, 2020 at 12:15 PM Brady Miller ***@***.***> wrote: It will cover the 'Loading' that is in the tab title, but not the 'Loading ...' that is in the tab frame. Still apparently stuck with a Loading (unless I am mistaken). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3079?email_source=notifications&email_token=AACCLQOHY2NFIIF5RQE5LUDRGZRQFA5CNFSM4LCEZHCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOMCTIY#issuecomment-597174691>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACCLQPRQ5ZFANPQQYQD43LRGZRQFANCNFSM4LCEZHCA> .

[sjpadgett]

Or we can just fix the underlying problem. xl(init)!

[bradymiller]

you mean actually fix the issue and not design and implements workarounds... gasp 😄

[kchapple]

Oh, I see why I see a blank screen, cause this PR is supposed to add a loader/spinner, haha. Why not just get rid of "loading" text. Everybody knows what the spinner means. I'm not sure what the underlying problem is, as described by Jerry, but I'm happy to jump on a call to see if I can help. Ken

…

On Tue, Mar 10, 2020 at 12:25 PM Jerry Padgett ***@***.***> wrote: Or we can just fix the underlying problem. xl(init)! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3079?email_source=notifications&email_token=AACCLQKCTVDLNXXBT5HYYHLRGZSY7A5CNFSM4LCEZHCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOMEBFA#issuecomment-597180564>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACCLQOSEMZFWDXVDTA6K4TRGZSY7ANCNFSM4LCEZHCA> .

[sjpadgett]

as described by Jerry

Problem is

openemr/interface/main/tabs/js/application_view_model.js

Line 15 in 73906bb

app_view_model.application_data.tabs=new tabs_view_model();

occurs before completes

openemr/interface/main/tabs/main.php

Line 222 in 73906bb

setupI18n(<?php echo js_escape($_SESSION['language_choice']); ?>).then(translationsJson => {

[sjpadgett]

The reason I say that, is because the instantiation of tabStatus() in the tabs_view_model() function can be removed (see my PR), and I think the "Loading" translation can be applied to the string using PHP in main.php around lines 254. There's no need for the javascript workaround.

@yashrajbothra I just merged Kens PR and it will help with this. PR #3124 it removes the auto instantiation of tab lists in tab_view_model where it was being instantiated anyway in main.php(I didn't even notice). So main.php is where you should be adding those spinners.

I'm guessing xl() should now work in all other places.

[sjpadgett]

if easier here is patch for PR
kens_pr3124.zip

[yashrajbothra]

Great then i will resolve the conflicts and test what changed and modify the PR accordingly :)

[tywrenn]

Just a thought, to make it easier for GSoC going forward, we should be putting this font-size: 80% or whatever the value is inside the SCSS instead of inline.

[yashrajbothra]

SCSS commit is pending will try to do it in morning. Ready to test.

[sjpadgett]

Hi @yashrajbothra I think we're almost there. I haven't tested my comments but i'm pretty sure they will complete this PR.

[sjpadgett]

just make fa-sm, fa-1x or remove completely and spinner will default to title font size. no need to mess with sizing it!!!

I can't believe we went through all these changes to just avoid adding one translated constant,'Loading' to script. Future dev's will look at this in code and say, what the heck. Why is this and why is that!!!!

[bradymiller]

hi @yashrajbothra , This is testing well and code looks good to me. Very nice work considering the 100+ posts on this PR :)
After @sjpadgett ok's this PR, then ready to come into codebase.

[sjpadgett]

thanks @yashrajbothra I will miss this PR:)

[yashrajbothra]

It feels good to see this PR merged 🎉. But don't you guys think the loading msg should be there whenever tab is reloaded and in general whenever any popup or dialogbox loads 🤔

[bradymiller]

hi @yashrajbothra , Definitely feel free to post PR(s) with more improvements on this front. You convinced us to use a spinner, so guessing you will convince us of more related improvements :)