feat: reduce props API surface area #162

frederickfogerty · 2018-07-30T01:00:27Z

BREAKING CHANGE: faces is no longer set by default.

Deprecations (supported until v9):

props auto, crop, entropy, faces, and fit (have been rolled into imgixParams)
customParams (renamed to imgixParams)

Description

This PR consolidates some imgix props into the imgixParams object, which makes the props API smaller and easier to understand. There should be no issues with excessive re-rendering as the object is checked shallowly to see if the object's values have changed. I decided to deprecate the old props rather than remove them to ease the transition to the new API. Any use of the old props will now trigger a warning, and we will remove the old props in v9.

In the past this library has set the faces imgix parameter by default. The new behaviour is to not set this parameter so that it should be less confusing for new-comers.

Also, I'm looking for good taglines/features about this library, so if you, reader, have any ideas, please let me know.

Ref #144

TODO

rename imgProps to htmlAttributes after deciding that imgProps and imgixParam are too similar
address PR comments

New Feature

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.
Run unit tests to ensure all existing tests are still passing
Add new passing unit tests to cover the code introduced by your PR
Update the readme
Add some steps so we can test your cool new feature!
The PR title is in the conventional commit format: e.g. feat(<area>): added new way to do this cool thing #issue-number
Add your info to the contributors array in package.json!

Steps to Test

Review changes to unit tests.

- roll auto, crop, entropy, faces, fit props into imgixParams - renamed customParams to imgixParams - faces default has been changed to false - old props have been supported until v9 through the use of a HOC

jayeb · 2018-07-30T21:49:15Z

src/react-imgix.js

-export default class ReactImgix extends Component {
+const defaultImgixParams = {
+  auto: ["format"],
+  fit: "crop"


If we're removing the fit=faces default, we need to also remove the fit=crop default. If it's left on, all images will crop to center instead of resizing as expected.

This should be noted as a breaking change as well, since people will need to re-add it in their imgixParams (née customParams) if they're intentionally using other crop modes.

As discussed offline, fit=crop works intuitively for developers, but it needs to be called out in the README somewhere.

jayeb · 2018-07-30T21:51:55Z

src/react-imgix.js

    disableSrcSet: PropTypes.bool,
    onMounted: PropTypes.func,
    sizes: PropTypes.string,
    src: PropTypes.string.isRequired,
    type: PropTypes.oneOf(validTypes),
    width: PropTypes.number,
    height: PropTypes.number,
-    disableLibraryParam: PropTypes.bool
+    defaultHeight: PropTypes.number,
+    defaultWidth: PropTypes.number,


I think defaultWidth and defaultHeight aren't supposed to be here. Weren't they removed in #159?

My bad, they were removed. Those pesky merge commits!

jayeb · 2018-07-30T21:57:15Z

src/react-imgix.js

+    if (params.crop) crop = params.crop;
+
+    let fit = false;
+    if (entropy || faces) fit = "crop";


I think we should just go all-in on auto-setting here: if the user provides any kind of crop value, we should automatically set fit=crop (otherwise crop= is always ignored by the painter).

if (!!crop) fit = "crop";

Usually I would object to this kind of "magic" but I can't think of a downside in this case.

This sounds like a great idea.

jayeb · 2018-07-30T22:00:58Z

src/react-imgix.js

+      ...this.props.imgixParams
+    };
+
+    const { faces, entropy } = params;


I feel a little bit uneasy about encouraging users to put "non-standard" parameters into the imgixParams object. I have no problem putting "helper" options into the parent this.props object, but if we're going to explicitly call this a map of "imgix params" then I feel like we should require users to pass in crop: 'faces' instead of faces: true.

I had actually forgotten that faces wasn't a top level imgix parameter. I agree with you, I feel uneasy about it as well. I'll remove faces in favour of using crop: 'faces' directly.

jayeb · 2018-07-30T22:02:00Z

src/react-imgix.js

      children,
      component,
-      customParams,


imgixParams should be added to this.props here, no?

I don't think so -- this is not setting the prop types for the component, this is destructuring props into local variables to make it easier to use in the render method. imgixParams is not used anywhere in render, only in ReactImgix.buildSrcs()

jayeb · 2018-07-30T22:07:44Z

src/react-imgix.js

+    DEPRECATED_PROPS.forEach(deprecatedProp => {
+      warning(
+        !(deprecatedProp in props),
+        `The prop '${deprecatedProp}' has been deprecated and will be removed in v9. Please update the usage to <Imgix imgixParams={{${deprecatedProp}: value}} />`


Following my above comment about not encouraging users to pass in { imgixProps: { entropy: true } }, this warning will need to be updated.

frederickfogerty · 2018-08-12T21:05:01Z

Hey @jayeb, I think I've addressed all the issues discussed - could you please give this another review?

@jayeb

…#163) BREAKING CHANGES: - picture and source types have been changed to components. ## Description **NB: This is currently a stacked PR. I will change the base to `version-8` when #162 is resolved. The last commit in the PR moved some stuff around, it may help to view [this diff](https://github.com/imgix/react-imgix/pull/163/files/0b88acb3f646691ef1df80b207b5e09935df54f5) first to see the feature changes.** This PR builds on the changes described in #144. Having the picture and source behaviour work using a `type` prop was causing `<Imgix />` to have mixed responsibilities. This was causing pecularities, such as needing to pass a `src` to a `<Imgix type='picture' />` even though it wasn't used. ### TODO - [x] Update Upgrade Guide - [x] Change imgProps to be htmlAttributes as discussed offline with @jayeb ~- [ ] Add prop fall-through behaviour to `<Picture />`~ ### 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 the changes to the tests.

* 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

frederickfogerty requested review from jayeb and modosc July 30, 2018 01:00

frederickfogerty mentioned this pull request Jul 30, 2018

v8 Breaking Changes Discussion #144

Closed

frederickfogerty changed the base branch from master to version-8 July 30, 2018 21:41

frederickfogerty added 7 commits July 31, 2018 09:42

test: add shallowUntilTarget helper

1d6c841

test: update tests for consolidated props and for HOC

4a6639a

feat: reduce props and wrap in HOC

5c6dde8

- roll auto, crop, entropy, faces, fit props into imgixParams - renamed customParams to imgixParams - faces default has been changed to false - old props have been supported until v9 through the use of a HOC

feat: ensure shouldComponentUpdate check arrays in imgixParams correctly

f9d711d

docs: update library description

797c936

fix: shallowEqual -> shallowequal

ced9da9

chore: delete unused comment

20b94f7

frederickfogerty force-pushed the fred/reduce-props branch from 09a1e78 to 20b94f7 Compare July 30, 2018 21:42

jayeb suggested changes Jul 30, 2018

View reviewed changes

frederickfogerty added 2 commits August 13, 2018 08:40

docs: add info about fit=crop default

ab8e655

feat: remove faces and entropy helpers

dc76240

frederickfogerty added 3 commits August 13, 2018 14:01

test: update deprecation tests to ignore warnings

d4ab3af

docs: remove deprecated props

5c1087a

chore: remove props.component usage

8b2463b

frederickfogerty mentioned this pull request Aug 13, 2018

feat: refactor picture and source behaviour into different components #163

Merged

9 tasks

jayeb approved these changes Aug 13, 2018

View reviewed changes

frederickfogerty merged commit 9fb0cb9 into version-8 Aug 13, 2018

frederickfogerty deleted the fred/reduce-props branch August 15, 2018 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reduce props API surface area #162

feat: reduce props API surface area #162

frederickfogerty commented Jul 30, 2018 •

edited

Loading

jayeb Jul 30, 2018

jayeb Jul 30, 2018

frederickfogerty Jul 30, 2018

jayeb Jul 30, 2018

frederickfogerty Jul 30, 2018 •

edited

Loading

jayeb Jul 30, 2018

frederickfogerty Jul 30, 2018

jayeb Jul 30, 2018

frederickfogerty Jul 30, 2018

jayeb Jul 30, 2018

frederickfogerty Jul 30, 2018

jayeb Jul 30, 2018

frederickfogerty commented Aug 12, 2018

feat: reduce props API surface area #162

feat: reduce props API surface area #162

Conversation

frederickfogerty commented Jul 30, 2018 • edited Loading

Description

TODO

New Feature

Steps to Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederickfogerty Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederickfogerty commented Aug 12, 2018

frederickfogerty commented Jul 30, 2018 •

edited

Loading

frederickfogerty Jul 30, 2018 •

edited

Loading