-
Notifications
You must be signed in to change notification settings - Fork 64
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: add container to render image as background behind children [WI…
…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.
- Loading branch information
1 parent
db53890
commit 5c3ecf6
Showing
10 changed files
with
910 additions
and
294 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,4 +10,4 @@ branches: | |
- /release.*/ | ||
|
||
node_js: | ||
- "10" | ||
- "9" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.