SmartSprites
  1. SmartSprites
  2. SMARTSPRITES-36

sprite-alignment should support center/middle option as well

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.2.3
    • Fix Version/s: 0.2.10
    • Labels:
      None

      Description

      Motivation

      The current list of supported values for the sprite-alignment property is incomplete: it does not cover the center value that is supported by the background-position CSS property. Adding center to the set of supported values will allow positioning sprite images at the center of the container. Original Jouni's report comes at the bottom.

      Implementation

      Add center to the set of values supported by the sprite-alignment property. If center alignment is specified for an individual image, the generated horizontal / vertical background-position CSS property will be center for vertically- / horizontally-laid out sprites. Additionally, the image will be centred within the width / height of vertical / horizontal sprites. The margins specified in sprite-margin-* will add up to the individual image being centered, which will allow shifting the individual image without adding extra padding to the image itself.

      Please see SMARTSPRITES-39 for more specification regarding interactions with sprite-width and sprite-height properties.

      Original report

      Currently the only option for for sprite-alignment are left | right | top | bottom | repeat.

      I have a use-case where I would need the option "center" as well (could be called "middle" as well).

      So when I write this in my original CSS like this
      {{
      /** sprite: verticals; sprite-image: url(../common/img/vertical-sprites.png); sprite-layout: vertical */

      background-repeat: no-repeat;
      background-position: center top;
      background-image: url(img/image.png); /** sprite-ref: verticals; sprite-alignment: center */
      }}

      it should be transformed to this
      {{
      background-repeat: no-repeat;
      background-position: center top;
      background-image: url(img/image-sprite.png);
      background-position: center -123px; /* (some arbitrary value for top) */
      }}

        Activity

        Hide
        Stanisław Osiński added a comment -

        Good catch, Jouni! If you have a chance to prepare a patch before I can get down to 0.2.4 issues (next week the earliest, I'm afraid...), feel free to attach.

        S.

        Show
        Stanisław Osiński added a comment - Good catch, Jouni! If you have a chance to prepare a patch before I can get down to 0.2.4 issues (next week the earliest, I'm afraid...), feel free to attach. S.
        Hide
        Jouni Koivuviita added a comment -

        I did fix this for my own build, but as soon as I was finished with it, I realized that it should support any arbitrary (pixel) value. I could as well write the following CSS:

        /** sprite: verticals; sprite-image: url(../common/img/vertical-sprites.png); sprite-layout: vertical */
        background-repeat: no-repeat;
        background-position: 42px top;
        background-image: url(img/image.png); /** sprite-ref: verticals; sprite-alignment: center */

        And it should be converted to (if the sprite is aligned to left in the sprite collection):

        background-repeat: no-repeat;
        background-position: center top;
        background-image: url(img/image-sprite.png);
        background-position: 42px -123px;

        And I now realize that the other images affect this as well. Supporting the "center" value is a bit more complex than I thought, since the image you specify to be centered might not be the widest in the sprite collection, resulting in misalignment. I guess any sprite that has "center" in sprite-alignment would have to be calculated last, so the position could be calculated correctly.

        Pixel values can be passed on to the final conversion as is, and the sprites positioned to the left of the collection.

        Any other issues I'm missing here?

        Show
        Jouni Koivuviita added a comment - I did fix this for my own build, but as soon as I was finished with it, I realized that it should support any arbitrary (pixel) value. I could as well write the following CSS: /** sprite: verticals; sprite-image: url(../common/img/vertical-sprites.png); sprite-layout: vertical */ background-repeat: no-repeat; background-position: 42px top; background-image: url(img/image.png); /** sprite-ref: verticals; sprite-alignment: center */ And it should be converted to (if the sprite is aligned to left in the sprite collection): background-repeat: no-repeat; background-position: center top; background-image: url(img/image-sprite.png); background-position: 42px -123px; And I now realize that the other images affect this as well. Supporting the "center" value is a bit more complex than I thought, since the image you specify to be centered might not be the widest in the sprite collection, resulting in misalignment. I guess any sprite that has "center" in sprite-alignment would have to be calculated last, so the position could be calculated correctly. Pixel values can be passed on to the final conversion as is, and the sprites positioned to the left of the collection. Any other issues I'm missing here?
        Hide
        Stanisław Osiński added a comment -

        Example use case of centering in vertical sprites.

        Show
        Stanisław Osiński added a comment - Example use case of centering in vertical sprites.
        Hide
        Stanisław Osiński added a comment -

        When it comes to centering in sprite images, I'm assuming your use case would be something like the (unsprited) example I've attached? That would indeed require offseting the individual image in the sprite in such a way that it's centered within the sprite. As far as I can remember the code, this shouldn't be too difficult, because the size of the resulting sprite image is calculated and known before the individual images are drawn to the sprite.

        When it comes to your second comment about arbitrary offsets – you can easily emulate that with sprite margin. Simply put sprite-alignment: left | right and set sprite-margin-left | right to your original position (42px in the example). The generated background-position offset will still be 0, but the original offset will be implemented within the sprite image, which will be offset by 42px from the left edge. Would that make sense?

        Incidentally, quite a long time ago I came up with an idea of more automatic inference of sprite properties based on the original background properties (SMARTSPRITES-1, I see you voted for it) which would handle these cases in a more elegant way. But that's a major refactor in the code, so I'd need to take a week of from my regular job to implement it

        Show
        Stanisław Osiński added a comment - When it comes to centering in sprite images, I'm assuming your use case would be something like the (unsprited) example I've attached? That would indeed require offseting the individual image in the sprite in such a way that it's centered within the sprite. As far as I can remember the code, this shouldn't be too difficult, because the size of the resulting sprite image is calculated and known before the individual images are drawn to the sprite. When it comes to your second comment about arbitrary offsets – you can easily emulate that with sprite margin. Simply put sprite-alignment: left | right and set sprite-margin-left | right to your original position (42px in the example). The generated background-position offset will still be 0, but the original offset will be implemented within the sprite image, which will be offset by 42px from the left edge. Would that make sense? Incidentally, quite a long time ago I came up with an idea of more automatic inference of sprite properties based on the original background properties ( SMARTSPRITES-1 , I see you voted for it) which would handle these cases in a more elegant way. But that's a major refactor in the code, so I'd need to take a week of from my regular job to implement it
        Hide
        Jouni Koivuviita added a comment -

        You're absolutely right Stanislav, I didn't think this through before commenting.

        I was able to find the right place in the code to make it work for myself again. And the sprite margins are absolute the right place to include any additional offsets, no need to do any modifications on that part. And it makes complete sense

        The automatic inference would be really nice, since it would reduce several lines of CSS per sprite. The length of the CSS I'm currently working on is exceeding 2300 lines already, so every byte that I can reduce is a small victory. But I realize it's a lot of work, so take your time.

        BTW, I might be willing to help you on the future enhancements of this great library, since I see it becoming quite an important part of my tool chain. But I'll try and submit these two patches for you first

        Show
        Jouni Koivuviita added a comment - You're absolutely right Stanislav, I didn't think this through before commenting. I was able to find the right place in the code to make it work for myself again. And the sprite margins are absolute the right place to include any additional offsets, no need to do any modifications on that part. And it makes complete sense The automatic inference would be really nice, since it would reduce several lines of CSS per sprite. The length of the CSS I'm currently working on is exceeding 2300 lines already, so every byte that I can reduce is a small victory. But I realize it's a lot of work, so take your time. BTW, I might be willing to help you on the future enhancements of this great library, since I see it becoming quite an important part of my tool chain. But I'll try and submit these two patches for you first
        Hide
        Stanisław Osiński added a comment -

        Moving out of the 0.2.4 release.

        Show
        Stanisław Osiński added a comment - Moving out of the 0.2.4 release.
        Hide
        Fabian Lange added a comment -

        There is a fix waiting for this in:
        https://github.com/carrotsearch/smartsprites/pull/7

        Show
        Fabian Lange added a comment - There is a fix waiting for this in: https://github.com/carrotsearch/smartsprites/pull/7
        Hide
        Stanisław Osiński added a comment -

        Implementation contributed by Artur.

        Show
        Stanisław Osiński added a comment - Implementation contributed by Artur .

          People

          • Assignee:
            Stanisław Osiński
            Reporter:
            Jouni Koivuviita
          • Votes:
            3 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 4h
              4h
              Remaining:
              Remaining Estimate - 4h
              4h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development