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

Add custom sizes for fluid images #8825

Merged
merged 17 commits into from
Oct 16, 2018
Merged

Add custom sizes for fluid images #8825

merged 17 commits into from
Oct 16, 2018

Conversation

nikoladev
Copy link
Contributor

This PR adds support for custom sizes when creating fluid images. The current implementation returns sizes in the following way:

num / 4   // if num is 800 --> 200
num / 2   // if num is 800 --> 400
num       // if num is 800 --> 800
num * 1.5 // if num is 800 --> 1200
num * 2   // if num is 800 --> 1600
num * 3   // if num is 800 --> 2400

With this PR you have direct control of the sizes you get back by using a new parameter called customSizes. See the example for how it works.

Example

Query

(For this example image.jpg has a width of 1000)

{
  fluid: file(relativePath: {eq: "image.jpg"}) {
    childImageSharp {
      fluid (customSizes: [ 50, 180, 400, 710, 900 ]) {
        src
        sizes
        srcSet
      }
    }
  }
}

Output

{
  "data": {
    "fluid": {
      "childImageSharp": {
        "fluid": {
          "src": "/static/image-f00db918cb54b413b814ed8e193478ad-97776.jpg",
          "sizes": "(max-width: 800px) 100vw, 800px",
          "srcSet": "/static/image-f00db918cb54b413b814ed8e193478ad-12166.jpg 50w,\n/static/image-f00db918cb54b413b814ed8e193478ad-3e6f0.jpg 180w,\n/static/image-f00db918cb54b413b814ed8e193478ad-7d9ea.jpg 400w,\n/static/image-f00db918cb54b413b814ed8e193478ad-97776.jpg 710w,\n/static/image-f00db918cb54b413b814ed8e193478ad-a0877.jpg 900w,\n/static/image-f00db918cb54b413b814ed8e193478ad-fd737.jpg 1000w"
        }
      }
    }
  }
}

Here's srcSet in a more readable format:

/static/image-f00db918cb54b413b814ed8e193478ad-12166.jpg 50w,
/static/image-f00db918cb54b413b814ed8e193478ad-3e6f0.jpg 180w,
/static/image-f00db918cb54b413b814ed8e193478ad-7d9ea.jpg 400w,
/static/image-f00db918cb54b413b814ed8e193478ad-97776.jpg 710w,
/static/image-f00db918cb54b413b814ed8e193478ad-a0877.jpg 900w,
/static/image-f00db918cb54b413b814ed8e193478ad-fd737.jpg 1000w

Some questions

Should maxWidth automatically be inserted into the customSizes array so that we have a perfect fit? Right now the nearest width to maxWidth is selected for src. (In the example above the 710w version is chosen for src.)

Is customSizes a good name for it?

options[fixedDimension] * 2,
options[fixedDimension] * 3,
]
: options.customSizes.slice()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to create copy of array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Turns out I was just being overly safe. I'll get rid of it in a future commit :)

@pieh
Copy link
Contributor

pieh commented Oct 5, 2018

Should maxWidth automatically be inserted into the customSizes array so that we have a perfect fit? Right now the nearest width to maxWidth is selected for src. (In the example above the 710w version is chosen for src.)

Yeah, I think so (if we aren't doing that somewhere already)

Also I think we need to do some additional validation if this is provided by user - we need to ensure that values are > 0 (unlikely, but I wouldn't be surprised if someone used that by mistake and that would probably produce weird result or weird errors).

Not sure here but maybe we need to sort values as well and maybe remove duplicates (not sure on browser specification for that).

Is customSizes a good name for it?

I don't love it, but don't have any nice alternative right now. Any arg we are passing would be "custom" - maybe "sizesBreakpoints" or something like that?

@nikoladev
Copy link
Contributor Author

This weekend I have some time and I'll implement the following changes:

  • Ensure maxWidth is added
  • Ignore negative values
    • Question: should negative values give some sort of warning/error? If so, how can I show those in the GraphiQL editor? (For the command line I assume that I can just use a console.warn/error, correct?)
  • Remove any duplicates
    • Sorting is already done a couple lines further: const sortedSizes = _.sortBy(filteredSizes)
  • Come up with a new name if inspiration strikes me. Otherwise I'll use sizesBreakpoints
    • If you think of something else in the mean time, please let me know :)
  • Remove unnecessary array cloning

@nikoladev
Copy link
Contributor Author

Some ideas for the name:

  • desiredSizes
  • specifiedSizes
  • indicatedSizes
  • requestedSizes
  • necessarySizes
  • requiredSizes
  • expectedSizes

What do you think @pieh?

@pieh
Copy link
Contributor

pieh commented Oct 5, 2018

  • Question: should negative values give some sort of warning/error? If so, how can I show those in the GraphiQL editor? (For the command line I assume that I can just use a console.warn/error, correct?)

I think (not 100% sure) that throwing error should show up both in console/terminal and in ___graphql it should show error(s) instead of data

Some ideas for the name:
What do you think @pieh?

I think that naming things is hard ;) we are not passing "sizes" really as an arg - just breakpoints for sizes (this got stuck in my head after I come up with it),

@nikoladev
Copy link
Contributor Author

@pieh Don't know if you'll already be notified of the update to this PR, but I'll ping you just in case :)

@pieh
Copy link
Contributor

pieh commented Oct 8, 2018

@nikoladev I thinks it's good right now, no further feedback/requests from me - will leave this open for ~1 day to get more feedback from community

@nikoladev
Copy link
Contributor Author

@pieh Any update on additional feedback?

@pieh
Copy link
Contributor

pieh commented Oct 16, 2018

@nikoladev there were no additional comments, let's merge it - I should merge it long time ago, heh

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nikoladev!

@pieh pieh merged commit 6cb4ee6 into gatsbyjs:master Oct 16, 2018
@fk
Copy link
Contributor

fk commented Oct 16, 2018

🙏 💜 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants