-
Notifications
You must be signed in to change notification settings - Fork 672
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
feature(views): added lazy loading of user hover menu #8054
Conversation
var $all_placeholders = $(".elgg-menu-hover[rel='" + $placeholder.attr("rel") + "']"); | ||
|
||
// find the <ul> that contains data for this menu | ||
var $ul = $all_placeholders.filter('[data-json]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make a better attribute name, elgg-menu-data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me
I'm really happy to see this on its way. |
|
||
$guid = (int) $user->getGUID(); | ||
$page_owner_guid = (int) elgg_get_page_owner_guid(); | ||
$contexts = (array) elgg_get_config("context"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$CONFIG->context
is no longer populated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was arguably a BC break in 1.10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can get this in first: #8060
Just reminding that #8060 has been merged and can now be used here. |
also waiting for #8057 |
updated to use hmac service and use the new context_stack functions. also changed the attribute name that holds the elgg-menu-data |
Scrutinizer errors only related to not receiving code coverage in a fair amount of time... |
// verify MAC | ||
$data = array($guid, $page_owner_guid, $contexts, $input); | ||
|
||
if (!elgg_build_hmac($data)->matchesToken($mac)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work because array arguments will be converted to "Array". elgg_build_hmac() should probably throw in that case. Easiest way to get this working:
$data = serialize([$guid, $page_owner_guidm $contexts, $input]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ow yeah...
I'll work my suggestions into a new PR |
* @return string[] | ||
*/ | ||
function _elgg_nav_public_pages($hook_name, $entity_type, $return_value, $params) { | ||
if (is_array($return_value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming stuff in hooks has bitten us in the ass a few times already
See #8128. I reworked the content view to more clearly and completely capture/restore state |
Can't say if this is working for me because in 100 members page all hover menus seem to load immediately, extremely fast....also tried with 200 members page no difference :S |
You should always see a loader (but just once for every single user per page). If you use firebug you should see ajax calls when clicking the user hover icon... |
I don't see any loader (do you mean the spinning icon right?) and hover menus are loaded right away (very fast anyway which doesn't happen in my 1.9.8 production site). |
i guess you are not using the correct branch/pr then... but this PR is also closed and work is continued in #8128 |
Maybe but git branch gives 1.x and origin is 1.x am I wrong? |
Hasn't been merged yet....right? :S |
1.x will only contain merged PR's. If you want to checkout someones PR, you need to add those to your git config |
Yeah....got it...what mislead me was that previous one (pagination) was merged already...ok will try to add Pawel code into git config to add all.... |
fixes #4495