codingjoe / django-pictures

Responsive cross-browser image library using modern codes like AVIF & WebP
BSD 2-Clause "Simplified" License
248 stars 20 forks source link

Allow DRF PictureField to accept aspect_ratios #54

Closed amureki closed 1 year ago

amureki commented 2 years ago

You may provide optional GET parameters to the serializer, to specify the aspect ratio and breakpoints you want to include in the response. The parameters are prefixed with the fieldname_ to avoid conflicts with other fields.

curl http://localhost:8000/api/path/?picture_ratio=16%2F9&picture_m=6&picture_l=4
# %2F is the url encoded slash
{
  "other_fields": "…",
  "picture": {
    "url": "/path/to/image.jpg",
    "width": 800,
    "height": 800,
    "ratios": {
      "1/1": {
        "sources": {
          "image/webp": {
            "100": "/path/to/image/1/100w.webp",
            "200": "…"
          }
        },
        "media": "(min-width: 0px) and (max-width: 991px) 100vw, (min-width: 992px) and (max-width: 1199px) 33vw, 25vw"
      }
    }
  }
}

Note that the media keys are only included, if you have specified breakpoints.

codecov[bot] commented 2 years ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (63adbc1) compared to base (266115d). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #54 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 9 9 Lines 260 276 +16 ========================================= + Hits 260 276 +16 ``` | [Impacted Files](https://codecov.io/gh/codingjoe/django-pictures/pull/54?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Johannes+Maron) | Coverage Δ | | |---|---|---| | [pictures/contrib/rest\_framework.py](https://codecov.io/gh/codingjoe/django-pictures/pull/54/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Johannes+Maron#diff-cGljdHVyZXMvY29udHJpYi9yZXN0X2ZyYW1ld29yay5weQ==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Johannes+Maron). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Johannes+Maron)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codingjoe commented 2 years ago

@amureki I added a commit that replicates the same API from the picture template tag. I have a feeling that might be more consistent. We could also consider having those attributes passed via GET parameters, so you can have a more dynamic REST API. Thoughts?

amureki commented 2 years ago

@codingjoe yeah, that looks better. I'd be fine moving forward with it!

codingjoe commented 2 years ago

We could even build a React and Vue component for the package, or at least an example, right?

codingjoe commented 2 years ago

@amureki I pushed an updates, that includes your suggestions. I am curious what you think.

amureki commented 2 years ago

@amureki I pushed an updates, that includes your suggestions. I am curious what you think.

Yeah, that's good and interesting idea with the GET params. I wonder if we should prefix those somehow to not clash with other possible API params that other dev could define in their APIs...

codingjoe commented 2 years ago

@amureki I pushed an updates, that includes your suggestions. I am curious what you think.

Yeah, that's good and interesting idea with the GET params. I wonder if we should prefix those somehow to not clash with other possible API params that other dev could define in their APIs...

Good idea, we should at least add the field name a prefix in case there are multiple picture fields. Do you think it'll be sufficient.

codingjoe commented 2 years ago

@chgad, we are proposing some changes to the DRF field. Since you suggested the original implementation. How do you feel about this?

codingjoe commented 2 years ago

cc @SebastianKapunkt what do you think?

SebastianKapunkt commented 1 year ago

It helps to reduce the payload of the request. To get the actual source you want to use in the frontend, you still need a JS snipped to extract what you need.

Maybe it makes sense to even add more parameters to the request, to the point where you only get one URL back.

codingjoe commented 1 year ago

Maybe it makes sense to even add more parameters to the request, to the point where you only get one URL back.

@SebastianKapunkt why would you want only one URL? You mean, if you already know all device information like pixel density and the final size? Pixel density is known per device, but even apps can be resized (e.g. via screen rotation). So, you'd never know the actual size.

To get the actual source you want to use in the frontend, you still need a JS snipped to extract what you need.

We wanted to keep the API payload as atomic as possible. Having a single srcset-string would be helpful in HTML only. That being said, a simple Array.join(', ') should be feasible for most.

@amureki maybe we should consider adding examples for React and Vue components to the docs?

amureki commented 1 year ago

@amureki maybe we should consider adding examples for React and Vue components to the docs?

56 is a good start. Then we could add some code examples for React and Vue to the docs as well. Should we also move the docs to readthedocs then?