[#8278] - Look ma no addScriptDeclaration
- Fixed in Code Base
- 2 May 2016
- Medium
- Build: staging
- Easy Test
- # 8278
- Diff
- dgt41:__isisJS
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
A better way to pass PHP vars to Javascript
Instead of using addScriptDeclaratio() which comes with much computation power we can use data attributes to pass all the vars. Then we collect them easily from the javascript.
As an example Isis passes two variables that control the toolbar buttons (if it is sticky and the height of the menu). Now these vars are injected in a null div and collected from the static script.
Testing
Apply the patch and play with the different options that isis provides in it’s configuration.
Votes
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
the data attribute should be in element for which it related, in current case it is for .subhead element, if I right ... then it will looks as nice use of data
Yep that’s better
I agree! Replacing the inline js that can be put in a external js file for HTML5 data-* attributes is the way to go. Especially in the frontend.
For instance, i think this (https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L667-L673) can be done with two data-* attributes in, for instance, body tag.
Example: <body ... data-keepalive-method="frontend" data-keepalive-time="360000">
And the js function moved to a joomla core js file but reading the values from body data-keepalive-* attributes.
@dgt41: sorry to hijack the PR but i think it's related with what you are doing.
@dgt41 yes this is better, but still can be better 
you still use id, and now search for it twice, at least 
and as here is jQuery already involved then no need to afraid of it 
here is quick suggestion:
...
<div class="subhead" data-tmpl-sticky="<?php echo $stickyBar; ?>" data-tmpl-offset="<?php echo $offset; ?>">
...var navTop,
isFixed = false,
$subhead = $('.subhead');
if ($subhead.data('tmplSticky')) {
processScrollInit();
processScroll();
$(window).on('resize', processScrollInit);
$(window).on('scroll', processScroll);
}
function processScrollInit() {
navTop = $subhead.offset().top - $subhead.data('tmplOffset');
// Fix the container top
$(".container-main").css("top", $subhead.height() + $('nav.navbar').height());
// Only apply the scrollspy when the toolbar is not collapsed
if (document.body.clientWidth > 480) {
$('.subhead-collapse').height($subhead.height());
$subhead.scrollspy({offset: {top: $subhead.offset().top - $('nav.navbar').height()}});
}
}
function processScroll() {
var scrollTop = $(window).scrollTop();
if (scrollTop >= navTop && !isFixed) {
isFixed = true;
$subhead.addClass('subhead-fixed');
// Fix the container top
$(".container-main").css("top", $subhead.height() + $('nav.navbar').height());
} else if (scrollTop <= navTop && isFixed) {
isFixed = false;
$subhead.removeClass('subhead-fixed');
}
}
@Fedik I agree for the use of a var instead of searching for the element all the time. I am not sure about the class, id’s were faster once in the world wild web
@andrepereiradasilva yes we are in the same line here. There are a lot of places that this can be applied in current joomla’s codebase
zero-24
- | Category | ⇒ | JavaScript Templates (admin) |
| Easy | No | ⇒ | Yes |
Tested all options in the ISIS template and all options can be altered and saved correctly. Test passed OK.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8278.
I have tested this item 
successfully on 5dc0f9a
Tested all options in the ISIS template and all options can be altered and saved correctly. Test passed OK.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8278.
I have tested this item successfully on 5dc0f9a
Tested all options in ISIS, all options work correctly.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8278.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
| Milestone |
Added: |
||
rdeutz
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-05-02 06:01:13 |
| Closed_By | ⇒ | rdeutz |
| Labels |
Removed:
?
|
||
no bad, but you use
hahaha
idand I not sure whether that it is good: to add extra element just for keep JS options ...
it doesn't sounds good ... it force one more DOM action "search for element" ...
you know that work with DOM always slow part of JS
how this idea will work when we have 100 small scripts?
we will get bunch of empty divs...
the
dataattribute should be in element for which it related, in current case it is for.subheadelement, if I right ... then it will looks as nice use ofdata