Skip to content
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

[ticket/13163] Fix responsive link lists #3032

Merged
merged 3 commits into from
Oct 17, 2014

Conversation

PayBas
Copy link
Contributor

@PayBas PayBas commented Oct 13, 2014

Rewrote the entire function, because it was incomprehensible spaghetti and a major resource hog (partly my own fault).

https://tracker.phpbb.com/browse/PHPBB3-13163

@PayBas
Copy link
Contributor Author

PayBas commented Oct 13, 2014

@hanakin @prototech @callumacrae could use some help with testing, since this is a blocker

@callumacrae
Copy link
Contributor

Why are you removing the global comment? That causes the code to fail JSHint.

@PayBas
Copy link
Contributor Author

PayBas commented Oct 13, 2014

@callumacrae you can simply set the global in your IDE. Doesn't need to be in the code.

@callumacrae
Copy link
Contributor

I can, but that doesn't make it correct :)

@prototech
Copy link
Contributor

There's a quirk in Chrome and Opera when refreshing at a width where an additional link would fit if a few more pixels were available. The script runs, does all its calculations, and settles on a set of links to display before the avatar loads; once the avatar loads, the link on the farthest right will drop to a second line.

@PayBas
Copy link
Contributor Author

PayBas commented Oct 15, 2014

From my own tests, usually if the avatar isn't loaded yet, the width of the "avatar" is extra wide (because it shows the alt text which is wider than the img). So the script will be extra conservative.

As for the "too conservative" behavior of the refresh... I've noticed this too. But as far as I could tell, this is about 10-20px, which should be within the acceptable limits (since its better to hide too much, rather than too few).

Nevertheless. I'll look into it.

@PayBas
Copy link
Contributor Author

PayBas commented Oct 15, 2014

Added

$linksAll.find('img').each(function() {
    $(this).load(function() {
        check();
    });
});

@nickvergessen
Copy link
Contributor

Looks fine to me now and works as intended now.
@prototech can you quickly review the JS code so we can merge this?

@prototech prototech merged commit 223ae1d into phpbb:develop-ascraeus Oct 17, 2014
@PayBas PayBas deleted the ticket/13163 branch October 17, 2014 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants