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

fix(gatsby-remark-images): remove clickable whitespace around image when linking #11528

Merged
merged 7 commits into from
Jun 24, 2019

Conversation

ajschmidt8
Copy link
Contributor

This PR changes the way that gatsby-remark-images handles linking image thumbnails to their originals.

Behavior Before PR

Currently, the a tag that links to the original image expands to fill the entire parent.
before

Behavior After PR

The new behavior makes the link only contain the image and not any extra whitespace.
after

The Jest snapshots for this package were updated to reflect this change.

@ajschmidt8
Copy link
Contributor Author

@pieh, @DSchau, is there anything I can do to get this reviewed/merged quickly?

@DSchau
Copy link
Contributor

DSchau commented Feb 8, 2019

@ajschmidt8 can you allow permission to push to your branch?

https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

To github.com:ajschmidt8/gatsby.git
 ! [remote rejected]     topics/fix-remark-images -> topics/fix-remark-images (permission denied)
error: failed to push some refs to 'git@github.com:ajschmidt8/gatsby.git'

@ajschmidt8
Copy link
Contributor Author

@DSchau, done!

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Is the general idea here that we don't want the extra space surrounding the image to be clickable?

I'm not really sure which behavior is preferable--could you provide why it's advantageous one way or the other?

@ajschmidt8
Copy link
Contributor Author

Is the general idea here that we don't want the extra space surrounding the image to be clickable?

I'm not really sure which behavior is preferable--could you provide why it's advantageous one way or the other?

@DSchau yes, that is the idea. The screenshots above are from a page on a site that I'm building. As indicated in the pictures, if you click in the whitespace surrounding the image, it will open the original image in a new URL. I found this to be kind of an annoyance. I thought it would be better if the original image only opened when the actual thumbnail was clicked and not the surrounding whitespace.

@ajschmidt8
Copy link
Contributor Author

It seems like it would be misleading to users if they clicked whitespace and an image popped up.

@DSchau DSchau changed the title Updates gatsby-remark-images link handling fix(gatsby-remark-images): remove clickable whitespace around image when linking Feb 8, 2019
@ajschmidt8
Copy link
Contributor Author

@DSchau, just pulled the latest and fixed some merge conflicts. let me know if this can be merged soon 😬

@LekoArts LekoArts added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label May 10, 2019
@freiksenet
Copy link
Contributor

@ajschmidt8 Could you allow me to push to your repo so that I can merge the conflicts?

@freiksenet freiksenet added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Jun 10, 2019
@ajschmidt8 ajschmidt8 requested a review from a team as a code owner June 10, 2019 23:58
@ajschmidt8
Copy link
Contributor Author

@freiksenet i just resolved the conflicts and pushed the changes 👍

@freiksenet freiksenet added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: awaiting author response Additional information has been requested from the author labels Jun 17, 2019
freiksenet
freiksenet previously approved these changes Jun 17, 2019
freiksenet
freiksenet previously approved these changes Jun 17, 2019
Copy link
Contributor

@freiksenet freiksenet left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Thank you for working on this.

@gatsbybot gatsbybot merged commit 8b398f4 into gatsbyjs:master Jun 24, 2019
@gatsbot
Copy link

gatsbot bot commented Jun 24, 2019

Holy buckets, @ajschmidt8 — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@wardpeet
Copy link
Contributor

Published in gatsby-remark-images@3.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants