User tests: Successful: Unsuccessful:
This PR drops the current custom code for sticking the two top toolbars together and instead use the already loaded bootstrap script!
Should work with all browsers but the idea was to fix #5970 and safari!
@wilsonge If this gets merged you got a nice conflict with 3.5-dev ???? Sorry!
@losedk @JoomliC and anyone with safari ever experienced this glitch can you give this a test?
Labels |
Added:
?
|
Hehe Wouldn't call them bugs, just minor issues. But let's get some people to test :)
@losedk actually this is a problem caused by safari’s rubber band, all other browsers are totally fine (with or without this PR). But also I think is better to use already loaded scripts (bootstrap) instead of writing our own custom ones.
And also with this PR you will never end up with a messed up screen
Category | ⇒ | JavaScript |
Status | New | ⇒ | Pending |
Easy | No | ⇒ | Yes |
@infograf768 JM can you test this with com_localize as I am touching the sidebar script here and AFAIK there were some compatibility issues when sidebar was introduced. Thanks
Status | Pending | ⇒ | Ready to Commit |
RTC based on Tests by @losedk and @infograf768 Thanks
Labels |
Added:
?
|
I just found another less intruding way to solve this:
html {
height: 100%;
}
That will not require a change for our scripts here
From a technical perspective I'd imagine it's better to use the bootstrap affix though than hack it ourselves and then add in fixes when the functionality already exists? I mean I'd rather get it right than bug fix the hacks :P
Apart from the wrong decision to affix also the sidebar (that’s a big change from the previous behavior and also is buggy), I totally agree with you.
Seems to fix the issue for me in 2 sites once I remembered to clear my cache.
Multiple good tests setting to RTC
@wilsonge would be good to get this into 3.4.2
Milestone |
Added: |
Milestone |
Added: |
Milestone |
Removed: |
We only fix critical things during an RC. As far as I understood this is only a minor visual thing.
I would not consider this minor. There are times when I am completely unable to get to buttons because the header never expands. It's enough to make me almost want to stop using Safari because at times the admin screens are unusable (can't name something, can't access the New button, etc).
No animation at all, just jumps... http://cl.ly/0s0v0i0e0S17
Milestone |
Removed: |
Milestone |
Removed: |
Milestone |
Added: |
Milestone |
Added: |
I do not quite agree with @mbabker that this is a step backwards, but I also realize that a better animation can be applied here. So with the latest commit I try to follow the same route Joomla did with sidebar: use css3 transition.
I would like to ask if you agree with this animation or if it is too long/short?
So @losedk @mbabker @brianpeat @infograf768 your feedback here is essential, Thanks
The animation is only triggering for me in Safari; Chrome and Firefox still just jump.
So here's why I say it's a step backwards. Right now, for the most part, the toolbar smoothly scrolls with the page and affixes when it reaches the target point. With this PR applied, scrolling one pixel causes it to jump up to its affixed spot. So in fixing an issue for one case, we're introducing a less than optimal behavior for another case.
FWIW, this same JavaScript logic is in use on the Bootstrap based joomla.org
template and there I can't duplicate the behavior in Safari of the page content not being below the second toolbar (in the .org case it's the site menu). Actually, when the page loads "properly", unpatched I see the body contents get pushed down during rendering in the Joomla admin, but the .org sites I tested against (API which is using pure HTML and an unaltered Bootstrap source and Developer which is a CMS install) don't exhibit the same behaviors.
The problem is that the current script used in Isis is somehow conflicting with the bootstrap collapse for the same toolbar. If you remove the collapse from the subhead everything works nicely. I think we have many competing scripts (sidebar, collapse, affix) and somehow there is an interference as all of them trying at the same time to calculate the save values e.g. height, width
Without redesigning the whole top section I don't see there being a good fix for this whole thing. Either something has to stop being affixed or we have to deal with rather ugly behaviors on scroll.
@mbabker messing around with the old script I found a root of the problem, so indeed we don’t have to drop the old script. Here it is:
function processScroll()
{
if ($('.subhead').length) {
var scrollTop = $(window).scrollTop();
if (scrollTop >= navTop && !isFixed) {
isFixed = true;
$('.subhead').addClass('subhead-fixed');
// Fix the container top
$(".container-main").css("top", $('.subhead').offset().top + $('nav.navbar').height());
} else if (scrollTop <= navTop && isFixed) {
isFixed = false;
$('.subhead').removeClass('subhead-fixed');
}
}
}
It is not having the expected behavior for me (just tested on FF), and with issue on mobile device (the last pixel up render smaller toolbar buttons)...
In the same time, #7342 is working nice, and don't change the behavior (i understand your goal with this PR, but not sure it's an easy job to introduce it in Isis (and not yet working with fluidity), or to keep some new stuff for a future new admin template ;-) )
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-07-07 14:01:50 |
Closed_By | ⇒ | wilsonge |
Milestone |
Removed: |
Labels |
Removed:
?
|
Seems to fix the problem :)
However noticed two things. The affix kicks in immediately on scroll and when clicking on menu items in the sidebar menu it jumps a bit