Hello,

It's generally better to use sprite. Here a patch for that. Old files sizes are 196 and 187 bytes. New file size (sprite) is 150 bytes. And sometimes 1 http request is saved.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Status: Active » Needs work

In this situation, I think we'll need to use a horizontal sprite rather than a vertical one. Either that or give it more vertical space. This patch would have a problem with large font sizes.

jcisio’s picture

Status: Needs work » Needs review
FileSize
153 bytes
899 bytes

New patch.

elachlan’s picture

What is the progress on this? It would be great to see it implemented.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks great, I'm just a bit busy and this is a minor issue. I'll commit it next time I'm working on Extlink.

quicksketch’s picture

Status: Reviewed & tested by the community » Needs work

Arg, bummer. In my testing this breaks the description text on admin/settings/extlink. We've tried to fix this in a dozen ways to no real avail. See #403786: Show themed icons on settings page.

elachlan’s picture

Now that issue is closed, has this been committed?

quicksketch’s picture

It's not closed, it's "needs work". No it hasn't been committed since it breaks the display on admin/settings/extlink.

jcisio’s picture

Status: Needs work » Needs review
FileSize
2.46 KB

Shameless try on settings page. It even could be better to remove the class and add a background-url, so that there is no problem if someone overrides the default css. However I'm not quite comfortable doing that with vi now.

I'm not trying to resolve #403786: Show themed icons on settings page, that has been just won't fixed.

elachlan’s picture

Tested and works.

Good work jcisio!!

I will leave the status changes to quicksketch

elachlan’s picture

@quicksketch - Does this fix the issue?

quicksketch’s picture

I'm not sure these changes are going to work across browsers as-is. I know the "inline-block" CSS is not supported by IE6 (not sure about IE7). Do you think anyone would really care if we just kept all 3 images? That way we could use the sprite image on the front-end and the individual images on the back-end and not worry about this problem at all.

jcisio’s picture

Yes, if it won't put more maintain work on your shoulders ;)

elachlan’s picture

The main reason for the change is performance, if the individual images are used in the backend, it doesn't matter as people don't care about that for normal users.

Dane Powell’s picture

Good idea, let's just make sure that the images are losslessly compressed as much as possible (I'm marking #1098186: extlink.png can be losslessly compressed as duplicate)

elachlan’s picture

Has this been implemented yet?

Dane Powell’s picture

Status: Needs review » Needs work

Would you mind re-rolling that patch git-style? Thanks.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.76 KB

Here's a D6 patch. Just rerolled as requested in August.

mgifford’s picture

Version: 6.x-1.x-dev » 7.x-1.12
FileSize
4.91 KB

Here's a D7 version too. I've added invisible text in so that it is as accessible as it can be. Sprites vs alt text is still an ongoing debate.

It works fine in Firefox, but not in Chrome for some reason.

elachlan’s picture

Bumping too see its inclusion.

elachlan’s picture

Version: 7.x-1.12 » 7.x-1.13
Status: Needs review » Needs work
Issue tags: -sprite

Patch to be re-rolled.

  • Administration interface to use normal images
  • Everything else will use the sprite
  • Alt text will be added using "title" attribute

See reference: http://www.w3schools.com/tags/att_global_title.asp

mgifford’s picture

Thanks @elachlan. Hopefully someone gets to re-rolling that patch.

Adding alt text via a title attribute is a bad idea. Many screenreader simply ignore titles, particularly on images.

Now if an organization like WebAIM.org was suggesting titles, that would be another issue, but w3schools does not have expertise in accessibility.

elachlan’s picture

Good call.

Will use what we already have then.

<div class="element-invisible">' + Drupal.t('External Links icon') + '</div>
mgifford’s picture

Yup. That will work better.

elachlan’s picture

Version: 7.x-1.13 » 7.x-1.x-dev
elachlan’s picture

Status: Needs work » Needs review
FileSize
1.35 KB

Re-rolled the patch with changes.

Status: Needs review » Needs work

The last submitted patch, extlink-963374-25.patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

Removed the sprite image file for now from patch.

elachlan’s picture

Status: Needs review » Patch (to be ported)

Commited to Git. Needs Port to 6.x and 8.x

DamienMcKenna’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Issue summary: View changes

Moving to the correct branch.

elachlan’s picture

Version: 8.x-1.x-dev » 6.x-1.x-dev

Looks like it's already implemented in 8.x-1.x-dev.

elachlan’s picture

Status: Patch (to be ported) » Closed (won't fix)