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

feat(gatsby-remark-images): set wrapperStyle as a function to enable dynamic css #12060

Merged
merged 10 commits into from
Mar 11, 2019

Conversation

CanRau
Copy link
Contributor

@CanRau CanRau commented Feb 25, 2019

Description

User can provide a function as wrapperStyle which receives the fluidResult containing image information like the aspectRatio which allows dynamic and customizable image styling

Old Description

Providing the new option addAspectRatio to the package settings, this PR will add the already available aspect ratio of the image to the wrapperStyle as flex property. Which allows images of different aspect ratios to adjust their sizes to match heights of siblings in a display: flex; container

like this
bildschirmfoto 2019-02-25 um 02 27 35
instead of this
bildschirmfoto 2019-02-25 um 02 28 33
live example: https://www.gaiama.org/15 (you'll have to scroll a little bit for images with differing aspect ratios)

I'm of course open to feedback and changes or other approaches 👍
IMHO it's a small change no one would ever notice not using it, but would help me a lot and maybe even open others the possibility of (in my opinion) beautiful inline image galleries 😀
I thought about a plugin running after gatsby-remark-images as well, yet that would mean recalculating the aspect ratios which are already present here.

By the way I'm currently running a small custom remark-plugin afterwards to wrap multiple images in the same paragraph in a flex enabled container. Link to old version where I overcomplicated things a little 🤦‍♂️https://github.com/GaiAma/gaiama.org/blob/master/packages/gatsby-remark-wrap-images/index.js so lines 31-41 is not necessary anymore.. new approach will be to just wrap images in a div with a customizable class, I could package that one up of course

Wish y'all a great monday 🙏

option.addAspectRatio adds the aspect ratio of the image as `flex: value;` to the wrapperStyle
@wardpeet
Copy link
Contributor

wardpeet commented Feb 25, 2019

Thank you for opening a PR! 💖 I'm not sure we want to introduce this functionality as this issue lies on the app level and not gatsby itself.

I would suggest making the wrapperStyle a function which gets the image information so it can be used to export a dynamic css string.

if (typeof options.wrapperStyle === 'function') {
  options.wrapperStyle = options.wrapperStyle(fluidResult);
}

unsure what the rest of @gatsbyjs/core thinks.

@sidharthachatterjee
Copy link
Contributor

Not sure if this belongs in gatsby-remark-images

@CanRau Have you tried styling the images using imageWrapperClass?

@CanRau
Copy link
Contributor Author

CanRau commented Feb 25, 2019

I would suggest making the wrapperStyle a function which gets the image information so it can be used to export a dynamic css string.

I like that 👍

UPDATE not yet sure why but it's only running the function on one image (first probably) then using that value for all others 🤔 so they all end up with the same aspectRatio
I use functions like this in other plugins successfully though

@CanRau Have you tried styling the images using imageWrapperClass

I need the image dimensions so it's not sufficient as far as I know. And as gatsby-remark-images already has all information necessary it would save a lot of processing.

like @wardpeet suggested, instead of hard coding the aspectRatio stuff we allow the user to provide a function which receives all necessary information
@CanRau CanRau changed the title [gatsby-remark-images] Add option to add the images aspect ratio to the wrapper as flex property [gatsby-remark-images] Let user provide function for wrapperStyle option to allow for dynamic styles Feb 25, 2019
@wardpeet
Copy link
Contributor

@CanRau If I need to take a look why you can't get it executed more than once, just let me know.

@wardpeet wardpeet changed the title [gatsby-remark-images] Let user provide function for wrapperStyle option to allow for dynamic styles feat(gatsby-remark-images): set wrapperStyle as a function to enable dynamic css Feb 25, 2019
@CanRau
Copy link
Contributor Author

CanRau commented Feb 25, 2019

@wardpeet thank you I already figured it out and fixed it, the new commits reflect all my changes
Had to declare a new variable otherwise it would override the pluginOptions overriding the function..

EDIT: let me know if the readme isn't clear enough or should follow another style, or any other feedback/changes 😉

@wardpeet wardpeet self-assigned this Mar 11, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

This looks solid! Thank you for your patience.

@CanRau
Copy link
Contributor Author

CanRau commented Mar 11, 2019

Yay 🎉 😁 glad you like it, thanks for approving, and thanks to me traveling the last ~2 weeks 😉

@wardpeet wardpeet merged commit d83f212 into master Mar 11, 2019
@wardpeet wardpeet deleted the CanRau-patch-1 branch March 11, 2019 10:17
@sidharthachatterjee
Copy link
Contributor

Published in gatsby-remark-images@3.0.7

@CanRau
Copy link
Contributor Author

CanRau commented Mar 16, 2019

Thanks for posting the release version, very useful!! 👍 🙏

CanRau added a commit that referenced this pull request Apr 18, 2019
I'm not entirely sure about this, I'm aware of the origin #3987 yet this PR I made proofs the opposite #12060 
Only thing I can think of right now that browser APIs get stringified, couldn't verify this so far, this #3834 is suggesting they get stringified and it's the reason the note was added to the docs in the first place..

Couldn't just leave my confusion alone and thought doing it in a PR might be useful if it's actually not valid anymore so it can be merged directly, otherwise feel free to reject it of course 👍
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