Add background mode back #160
Labels
Comments
|
This is essential for our project. So we need to use an old version of react-imgix currently. Could use |
|
Due to the community desire to have this reimplemented, this is happening! I'll post any updates here when I have them 👍 |
14 tasks
|
We're getting very close now! Track the progress over in #236.
|
frederickfogerty
added a commit
that referenced
this issue
Dec 21, 2018
…P] (#236) ## Description This PR adds a new React component which enables an image to be rendered as a background behind children components. This used to exist in this library before a previous update removed it. Because of the community support in #160, it has been re-implemented. I've opened this PR early to gain feedback. ### TODO - [x] add ref support - [x] Pull jsx rename out into a separate PR - [x] Change "around children" - [x] Round dpr to 2dp - [x] Make findNearestWidth more efficient - [x] Investigate adding bgSize to imgix URLs. - [x] Update findNearestWidth ### Discussion Points - Should `width` and `height` be top-level props (or just props in `htmlAttributes` and `imgProps`)? This was available in the old implementation, so it'd be a +1 to be compatible with that. This is at odds with the strategy taken by this library currently which is to try limit the number of top-level props. - Are there any tests I should add (in addition to the ones I have already added/stubbed in)? - Should there be an `onMounted` prop? We have a `onLoad` for our `img` tag, but this only makes sense because there is an equivalent attribute for a native `img` tag. I'm thinking we shouldn't. - Should we scale up the background image by the current dpr of the screen? - Shall the width of the background image be snapped to the nearest width available from our srcset widths? This would add the benefits that we get from that feature to this feature, but it could also make the prop configuration confusing. For example, if the user supplies a height, do we render an image with that height and calculate a width for that height, or do we change the height so it will match a srcset-width? ### New Feature - [x] If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted. - [x] Run unit tests to ensure all existing tests are still passing - [x] Add new passing unit tests to cover the code introduced by your PR - [x] Update the readme - [x] Add some [steps](#steps-to-test) so we can test your cool new feature! - [x] The PR title is in the [conventional commit](#conventional-commit-spec) format: e.g. `feat(<area>): added new way to do this cool thing #issue-number` - [x] Add your info to the [contributors](#packagejson-contributors) array in package.json! ## Steps to Test Review changes to unit tests.
|
🎉 Released in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Before you submit:
Is your feature request related to a problem? Please describe.
I would like to set images as the background image of components such as
divso I can place other items within this element.Describe the solution you'd like
We could make this work by requiring
widthandheightto be specified, as determining this ourselves is too error prone.The text was updated successfully, but these errors were encountered: