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: warn the user when no <img> passed as a child to <picture> fixes #90 #151

Merged
merged 3 commits into from
Jul 3, 2018

Conversation

frederickfogerty
Copy link
Contributor

@frederickfogerty frederickfogerty commented Jul 1, 2018

BREAKING CHANGE: A fallback image will no longer be created when using react-imgix in picture mode

Description

The fallback image support when using react-imgix in picture mode was causing issues with sizing in #90 as the size being passed to the root picture component was being used for the fallback as well. This was causing problems, and the simplest solution was to remove the fallback image creation. This is inline with the HTML spec, so anyone used to using <picture> elements in HTML will be at home with this. There is more discussion in #90.

A problem here is that src is a required prop for react-imgix, which will cause an error when using picture mode as a src now isn't going to be passed to the root component. An option we have here is to update the src prop to not be required, and just to warn the user when a src is not passed when the component is being used in img, bg, or source. Another option is to wait to see if we implement the splitting of components as described in #144. In either way this needs to be discussed and resolved before this PR is merged.

Also, since this is a breaking change, I'm going to merge this into the version-8 branch, and I'll look to land all the breaking changes at once (e.g. the ones being discussed in #144).

This PR fixes #90

Bug Fix

  • All existing unit tests are still passing (if applicable)
  • Add new passing unit tests to cover the code introduced by your PR
  • Update the readme
  • Update or add any necessary API documentation
  • Add some steps so we can test your bug fix
  • The PR title is in the conventional commit format: e.g. fix(<area>): fixed bug #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

Observe the changes to the unit tests in test/unit/react-imgix.test.js

@frederickfogerty frederickfogerty requested review from modosc and jayeb and removed request for modosc July 1, 2018 08:46
@frederickfogerty
Copy link
Contributor Author

frederickfogerty commented Jul 3, 2018

@jayeb I would appreciate your thoughts on the problem described above ☝️🙌

@jayeb
Copy link

jayeb commented Jul 3, 2018

The component-splitting you brought up in #144 seems like a good idea on its own, and if it also happens to solve our problem here then that's just gravy.

@frederickfogerty
Copy link
Contributor Author

Great, thanks @jayeb. I'm going to merge this into version-8 now, and the problem will be resolved when we implement component splitting.

@frederickfogerty frederickfogerty merged commit aab9358 into version-8 Jul 3, 2018
@frederickfogerty frederickfogerty deleted the fred/fix-90-new branch July 3, 2018 21:47
frederickfogerty added a commit that referenced this pull request Jul 23, 2018
* version-8:
  fix: warn the user when no <img> passed as a child to <picture> fixes #90 (#151)

# Conflicts:
#	src/react-imgix.js
#	test/unit/react-imgix.test.js
frederickfogerty added a commit that referenced this pull request Aug 15, 2018
* version-8:
  docs: add link to fluid example and fix some links
  docs: add example about fitting to the size of the container
  docs: add ref information
  feat: refactor picture and source behaviour into different components (#163)
  feat: reduce props API surface area (#162)
  feat: implement responsiveness with srcSet and sizes (#159)
  fix: warn the user when no <img> passed as a child to <picture> fixes #90 (#151)

# Conflicts:
#	README.md
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.

2 participants