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/12911] convert imageset to sprite #2794

Closed
wants to merge 26 commits into from

Conversation

Projects
None yet
5 participants
@hanakin
Copy link
Member

hanakin commented Aug 3, 2014

convert all the forum/topic imageset icons into a single sprite to reduce server requests and overall file size

TODO:

  • Chrome
  • Firefox
  • Safari
  • IE
  • IOS
  • Android
  • Fix topic icon

PSD: https://www.dropbox.com/s/l5dq1kdmx7486zt/imageset.psd
Demo: http://12911.midaym.com/viewforum.php?f=2&sid=4d651a2a4d14afd7d883392bbc4985cc
Ticket: https://tracker.phpbb.com/browse/PHPBB3-12911

PHPBB3-12911

@hanakin hanakin changed the title [WIP] [Ticket/12911] convert imageset to sprite [Ticket/12911] convert imageset to sprite Aug 3, 2014

@@ -27,7 +27,7 @@

<!-- IF not forumrow.S_IS_CAT -->
<li class="row">
<dl class="icon {forumrow.FORUM_IMG_STYLE}">
<dl class="row-icon {forumrow.FORUM_IMG_STYLE}">

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Aug 3, 2014

Contributor

is it required to break all existing templates?

@@ -1,3 +1,149 @@
.row-icon:before {

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Aug 3, 2014

Contributor

I'd prefer topiclist dl.icon:before, forumlist dl.icon:before

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 3, 2014

Author Member

yes because icon is used for the header and the rows so one of them needs changed if not both of them. I can add the icon back but row-icon is required

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Aug 3, 2014

Contributor

Nah, that's not helping.
You still break all forum and topic lists until they add the new class.

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 3, 2014

Author Member

The problem is that the topiclist is horribly written from a semantic standpoint leading to countless excessive unnecessary styles spidered throughout the code base. As well as cross styling issues which require chaining several dom nodes thus slowing down the render time

hanakin added some commits Aug 3, 2014

[ticket/12921] Remove obsolete image references
Contact icons is now a sprite, no need for
old includes

PHPBB3-12921
[ticket/12911] Fix functional upload test
Test should be on something less likely
to change like the site logo in the docs

PHPBB3-12911
@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 4, 2014

.row dl.icon:before {
margin: 2px 8px;
line-height: 1;
vertical-align: text-bottom;

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Aug 4, 2014

Contributor

some are tabs (right) some spaces

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 4, 2014

Author Member

@Louis7777
No ie8 transformations but who cares it's an enhancement for those that do and degrades for ie8 which makes up for 1% of browsers

This comment has been minimized.

Copy link
@Louis7777

Louis7777 Aug 4, 2014

Contributor

It certainly is much more than just 1% at the moment. Even so, isn't it too much of CSS code just to make an icon jump up and down?

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 4, 2014

Author Member

@Louis7777 ok 6% but its still irrelevant. Try this new animation out and see how you like it
@nickvergessen fix the tabs vs spaces issues

hanakin added some commits Aug 4, 2014

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 4, 2014

@Louis7777 your right about the moz the site I got the idea from had it but I forgot to remove when i copied it over. As for the IE again It does not matter as both IE8 & 9 are both floating around 6% and its a progressive enhancement. Animated gifs are a no go as the point is to use sprites to reduce bandwidth and server requests.

As for retina its for all high res mobile devices not just apple products but these are one of the dominate device so it has to be accounted for.

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented Aug 4, 2014

@hanakin shouldn't you also remove all -moz-animation, -moz-transform and -o-transform?

Your demo site's animations & icons look fine for me using the latest Firefox and Chrome browsers on Android smartphone and tablet:

  • Sony Xperia S with Android v4.1.2 (Jelly Bean)
  • Samsung Galaxy S T805 with Android v4.4 (KitKat)

I also tested the native browser on the smartphone and it doesn't display the animation, but other than that it's fine.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 5, 2014

@Louis7777 I ran it through pleeese.io should have all the proper prefixes now please retry it on android as I think native required w3c box-shadow

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented Aug 5, 2014

1. I tested it on my smartphone's native browser again (Sony Xperia S with Android v4.1.2 (Jelly Bean)) and it doesn't show the animation, but I think it is a problem on the specific device (although I cleared the cache, data etc.). Firefox and Chrome for Android on the same device show it just fine.

2. I tested it on the tablet (Samsung Galaxy S T805 with Android v4.4 (KitKat)) and everything's fine.

3. I tested it on another smartphone - LG g2 d802 with Android v4.4.2 (KitKat), the animation shows and all is fine.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 5, 2014

you need version 3+ on android so it should work on jelly bean

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented Aug 5, 2014

I've also tested this on latest Opera for Windows, and everything's fine.

However, there seems to be a small problem with Internet Explorer 11. The hot icon moves from its initial position and back for every pulse - I think 1 pixel to the left and 1 to the top.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 5, 2014

interesting I do not have an Ie11 image at the moment have to download one but can you check both the read and unread icons and verify the same result?

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented Aug 5, 2014

The unread hot topic animation and icon are fine in IE11. Only the read icon is moving.

However, by staring at the hot unread's animation, I'm not sure whether the maximum pulse's circle has the same center as the icon (not concentric). I think there's something weird about it, but maybe it is just my eyes playing tricks because when I zoom in on the icon the circles seem to be concentric.

Also, the icons are missing the titles and they are not clickable at your demo site. I assume you are aware of it, but I'm telling you just in case. :P

hanakin added some commits Aug 6, 2014

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented Aug 6, 2014

Another thing - the forum_unread_subforum image is not the correct one.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 6, 2014

hmm seems i forgot to turn the circle back on in the psd

hanakin added some commits Aug 6, 2014

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 6, 2014

So as it turns out Ie just does not support animations properly in any version sort of 10 but not really. 11 has a weird jitter and 10 has an odd offset at the beginning of the animation. Can MS just stop making software they absolutely sux at it! ok rant over

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 6, 2014

weird cached it maybe either way should be good now

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented Aug 6, 2014

Alright, I've rechecked everything.

  • Chrome, Firefox, Safari and Opera for Windows. 👍
  • Chrome and Firefox for Android. 👍
  • Android native browser on a smartphone and a tablet. 👍

Which leaves us with Internet Explorer:

  • IE8 and IE9 don't support transitions and keyframes (personally I'm fine with that :P). 👎
  • IE10. What is that odd offset that you previously mentioned? Have you fixed it?
  • IE11. Hot unread animation and icon seem fine (I still can't decide whether the circles are concentric or if my eyes are playing tricks). 👍
  • IE11. Hot read animation is fine but the icon moves up 1 pixel and back for every pulse (it is better than before since it doesn't move to the left anymore). 👎

EDIT: I've just spotted another unwanted behaviour. Using the latest Firefox on Windows. If you mouse over (highlight) or mouse out of the topic row or the hot icon itself, the animation is interrupted and restarts.

@bantu bantu added the 3.2 (Rhea) label Aug 22, 2014

@nickvergessen nickvergessen modified the milestone: 3.2.0 Oct 22, 2014

@naderman naderman force-pushed the phpbb:develop-ascraeus branch from 61ad400 to 0e772af Nov 2, 2014

@Louis7777

This comment has been minimized.

Copy link
Contributor

Louis7777 commented May 15, 2015

I've used an online image optimizer to reduce the size of the imageset.png down to 5,02 KB

imageset-min

@callumacrae

This comment has been minimized.

Copy link
Contributor

callumacrae commented May 28, 2015

Looks good 🎉

Doesn't this make it really tricky for other people to add images? I'd prefer it if we had a tool that we could use to automatically generate the spritesheet and CSS.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented May 28, 2015

lets not rehash that conversation again. lets just convert to svgs for these in 3.2 or use font-awsome...

@callumacrae

This comment has been minimized.

Copy link
Contributor

callumacrae commented May 29, 2015

Haha, I wasn't there for the first conversation. Close the PR, then?

@hanakin hanakin closed this May 29, 2015

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Sep 4, 2015

@callumacrae i know we said use svgs and I think thats still the better approach. but that will still make the imageset difficult to replace as the best method would be to use the <svg><use></svg> #3880

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.