Skip to content
This repository has been archived by the owner on Aug 10, 2022. It is now read-only.

UX improvements #23

Merged
merged 5 commits into from Feb 17, 2016
Merged

UX improvements #23

merged 5 commits into from Feb 17, 2016

Conversation

dgrammatiko
Copy link
Contributor

Few improvements for better UX

What is changed:

  1. Removed from the javascript all the static styling and put it in the relative css/class
  2. Added css class body.ifw-busy to prevent scrolling of the page
  3. Apply and remove the class ifw-busy when a request is made and an answer is received
  4. this PR also replaces Hide the animation when changing tab #7 with the suggestion of @gunjanpatel! Thanks!

Testing

Make sure you have web installer installed!
Download and replace:
plugins/installer/webinstaller/css/client.css
plugins/installer/webinstaller/css/client.min.css
plugins/installer/webinstaller/js/client.js
plugins/installer/webinstaller/js/client.min.js

Whenever you click on any link in the iframe you should see an overlay and a spinner
Try to scroll when spinner is shown, (no scrolling should happen)
Try to change tab when the spinner is shown (the new tab should show up correctly)

Preview

screenshot 2015-05-23 17 16 56

.css("z-index", "1000")
.css("opacity", "0.80")
.css("-ms-filter", "progid:DXImageTransform.Microsoft.Alpha(Opacity = 80)")
.css("filter", "alpha(opacity = 80)")
.appendTo('#myTabContent');

Choose a reason for hiding this comment

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

Thanks @dgt41
Tested:
It's not working like expected. Joomla loader is not hiding on other tabs.
installer-joomla-loader


It seems that you missed to include #web in following code.

.appendTo('#myTabContent #web');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damn copy paste, will push it now, Thanks!

@gunjanpatel
Copy link

@dgt41 I sent PR https://github.com/dgt41/install-from-web-client/pull/1 to fix it.

@peterlose
Copy link

@dgt41 Just tested it. I'm getting an error: client.js:62 Uncaught ReferenceError: apps_j25 is not defined

@dgrammatiko
Copy link
Contributor Author

@losedk Thats strange as apps_j25 is declared elsewhere, in the plugin itself https://github.com/joomla-extensions/install-from-web-client/blob/master/plugins/installer/webinstaller/webinstaller.php#L103

@gunjanpatel
Copy link

may be not because syntax is wrong? I guess should be

var apps_j25 = $j25;

@dgrammatiko
Copy link
Contributor Author

@gunjanpatel You are right, vars should be declared properly

@gunjanpatel
Copy link

👍

@peterlose
Copy link

That fixed the js issue. I'm just gonna do some more testing

@nicksavov nicksavov mentioned this pull request Jun 1, 2015
@nicksavov
Copy link
Contributor

@dgt41 , looks like there are some merge conflicts. Could you re-sync?

@dgrammatiko
Copy link
Contributor Author

@nicksavov resynced

@beat
Copy link
Contributor

beat commented Jun 11, 2015

Re-synced code-changes look ok to me.
We need 1-2 testers for the resynced PR here before merging.

@dgrammatiko
Copy link
Contributor Author

@gunjanpatel @losedk @nicksavov Can you retest this, so it can finally gets merged?

@nicksavov
Copy link
Contributor

I think we should just merge it, since it's a resync.

apps_updateavail1 = '$updatestr1',
apps_updateavail2 = '$updatestr2',
apps_obsolete = '$obsoletestr',
apps_j25 = $j25;

Choose a reason for hiding this comment

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

It seems this line is generating issue. It is look like that heredoc is not able to understand that this is JS variable.
Having issue like below with the latest changes.
web-intaller

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's because https://github.com/joomla-extensions/install-from-web-client/pull/18/files#diff-cc215dbf1ed448beb9f785b4a2f56531L87 was removed. The PR needs updating and retesting, unfortunately.

@beat
Copy link
Contributor

beat commented Jul 4, 2015

@dgt41 It looks that this one is not ready for merging according to latest tests from @nicksavov ?

Can you please check ?

@dgrammatiko
Copy link
Contributor Author

@beat Everything should be synced and ready to go now!

@beat
Copy link
Contributor

beat commented Jul 8, 2015

@dgt41 Thanks for the latest commit

We now need 1-2 successful tests and confirmation that it now fixes the issue reported by @gunjanpatel above. Any takers ? @gunjanpatel @nicksavov @losedk

@gunjanpatel
Copy link

Tested Successfully.
Working nice with latest patch.
Tested on FF and Chrome on Ubuntu.
Need 1 more test.
Thanks @dgt41 and @beat

@smz
Copy link

smz commented Jul 11, 2015

#test OK, everything as expected.

Thanks!

@beat
Copy link
Contributor

beat commented Jul 12, 2015

I wanted to merge this PR, and did a last third test on it, but I found an issue: The nice Joomla spinning wheel doesn't display anymore during initial loading.

To reproduce (i did it on Firefox 39 (latest):

  1. checkout master, go to install from web page, refresh page: Text "Loading..." and Joomla spinning logo shows.
  2. checkout UX branch of dgt41, clear cache, refresh page: Only text "Loading..." shows up.

@dgt41 can you please check what happened to the spinning Joomla logo ?

@gunjanpatel
Copy link

@beat On FF 39 linux. Tried to clear catch and reproduce the issue which you mention. But not able to reproduce it. I am getting "Loading..." and Joomla spinning logo both.

@beat
Copy link
Contributor

beat commented Jul 13, 2015

@gunjanpatel just checked with 2 different servers (http localhost and remote https server) and 2 different browsers (Firefox 39 and WebKit 2.4.8-based Epiphany 3.10.3): The spinning Joomla logo does not display with "Loading..." when initially loading the install-from-web page. So not ready for merging yet.

@gunjanpatel
Copy link

Agree if it not working for different environment then definitely need fix.
Cleared catch. Installed new staging site and get patch from this PR. Only I am not able to reproduce it so far. :( weird.

@dgrammatiko
Copy link
Contributor Author

@gunjanpatel @beat I’ve updated this one so that the spinner is visible, by simplifying part of the script and with some css additions. Should work flawlessly now for every browser

@beat
Copy link
Contributor

beat commented Feb 17, 2016

Found the reasons I was not seeing the spinner: There are 2 reasons!

  1. My Joomla install is in a subfolder, and that case doesn't get taken in account by this PR.

  2. I was on Joomla 3.5 master which has probably by mistake moved the ajax spinner image to a new folder!

Commented here:
joomla/joomla-cms@87db28b#commitcomment-16148637

For reason number 2, I believe it's a Joomla master issue, will do a separate PR to joomla-cms.

For reason number 1, it is a fool-proof easy fix for which i'm making a separate PR.

So: Good to merge for me too! so, I'm merging.

beat added a commit that referenced this pull request Feb 17, 2016
@beat beat merged commit 8b30737 into joomla-extensions:master Feb 17, 2016
@beat
Copy link
Contributor

beat commented Feb 17, 2016

@dgt41 @gunjanpatel @nicksavov Can any of you also please quick-review and comment on my PR #41 ? should take less than a minute as it's obvious (I tested it here already in a subfolder install)

@beat
Copy link
Contributor

beat commented Feb 17, 2016

Thanks to 2 reviews, merged #41 too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants