The docs / definition for hook_rrssb_buttons implies that the SVG should be embedded directly in the module file (inline in the associative array), rather than loaded from a separate file. Unfortunately, this has lead some developers to assume that this is good style -- embedding large chunks of SVG in the PHP file, which just makes it more difficult to update the SVG when it needs to be customized.

#2490828: Reduce page size and improve caching requested that we not embed SVGs in the output HTML, but rather to reference external SVG resources. My request is different -- I'd like some guidance from the module developers to indicate that the SVG doesn't have to be embedded into the PHP code. It's fine to use something like file_get_contents() to get the code to place in the hook.

Ideally, I think that it would be great if the hook definition worked more like drupal_add_js() where you have the option of embedding the SVG in what gets passed back OR you can provide a filename that the module will load.

Comments

GuyPaddock created an issue. See original summary.

GuyPaddock’s picture

Issue summary: View changes
GuyPaddock’s picture

Issue summary: View changes
AdamPS’s picture

Thanks for raising. I am interested in feedback/discussion.

I think there are two distinct questions. I think they can be considered separately because the RRSSB module code can convert between 1) and 2). Please can you have a think about this separation, and let me know which part you are mostly concerned with?

1) How should the website owner/admin specify custom icons. In the stable version it is only possible by modifying the module code. In the dev, there is a hook. Even better would be to allow it through the UI.

Yes if you want to call file_get_contents in the hook that seems fine to me. The results are cached so it would be efficient. Of course if you update the file, don't forget to clear the cache.

2) How should the server deliver icons to the browser/user - which impacts on performance/efficiency for the user. Stable and dev use inline SVG. As the linked issue notes, this increases page size and is less good for caching.

My current idea is to use background-images defined in an auto-generated CSS file. This means no extra files, efficient caching, minimal download size and cross-browser compatibility. It doesn't support further conditional styling of the icons in CSS (which is available now, although probably not commonly used) - and so doesn't support the "micro" icons where the background and foreground colors flip. To my mind such icons are too small for usability, but I guess opinions may vary. I have working code for this, but didn't quite find time to tidy up and check in to dev.

There is a partially complete dev V2 of the rrssb library that has js to background load and inject from an external SVG file per icon. This brings the overhead of fetching multiple separate files, and larger css/js files. It will be slower in the browser, although with a modern PC and fast connection, probably can't notice. NB the library V2 is optimised for manual integration into a page, but seems less optimised for integration into a CMS.

My idea number two is to use a single external SVG (cam be auto-generated again) with all the configured icons. This one is potentially trickier for browser compatibility.

Let me know any thoughts you have.

AdamPS’s picture

Title: Discourage embedding of SVG in code » Read SVG from file, use library icons
Component: Documentation » Code
Status: Active » Fixed

I've fixed this as follows:

  • hook_rrssb_buttons can specify an SVG file as an alternative to specifying SVG in-line.
  • By default, RRSSB module looks for an SVG file in the library 'icons' directory matching the button name.
  • Change the default hook to use the library icons.

  • AdamPS committed 9c09881 on 7.x-2.x
    Issue #2668466 by AdamPS: Read SVG from file, use library icons
    

  • AdamPS committed fca69da on 7.x-1.x
    Issue #2668466 by AdamPS: Read SVG from file, use library icons
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.