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

<Background> generating parameters twice for width and height #763

Closed
JorgeSivil opened this issue Mar 1, 2021 · 5 comments · Fixed by #780
Closed

<Background> generating parameters twice for width and height #763

JorgeSivil opened this issue Mar 1, 2021 · 5 comments · Fixed by #780

Comments

@JorgeSivil
Copy link

Based on the code I had to send imgixParams{ w: X, h: Y } to use the forced values.

But this would result in two parameters w and h appended to the URL.

Expected result: Only one parameter should be sent for a given key.

@sherwinski
Copy link
Contributor

Hey @JorgeSivil, thanks for bringing this issue to our attention. Our team will look into this and get back to you here. We'd also be happy to accept a PR that fixes this issue if anyone is interested in opening one.

@ericdeansanchez
Copy link
Contributor

ericdeansanchez commented Apr 1, 2021

Reproduced.

Steps to reproduce:

  • create react app
  • use the Background component like so:
import { Background } from "react-imgix";

function App() {
  return (
    <Background
      src="https://assets.imgix.net/dam-marketing/Updates_2020_Launch/Summer/Image_Tags.png?fm=jpg&q=90&ixlib=imgixjs-3.4.1"
      className="blog-title"
      imgixParams={{ w: 100, h: 100 }}
    >
      <h2>Blog Title</h2>
    </Background>
  );
}

export default App;

Options:

  • we could change how this works here (e.g. transform params that need it and letting "the last width/w win")
  • there's also this here, but...
  • of course, there are other options; there may be problems with the first suggestion (i.e. what if the wrong width wins?)

Test to catch this error:

    describe("when both w and h provided", () => {
    it("does not duplicate query params for height and width", () => {
      const sut = renderIntoContainer(
        <Background
          src="https://assets.imgix.net/examples/pione.jpg"
          imgixParams={{
            w: 300,
            h: 350,
          }}
          className="bg-img"
        ></Background>
      );

      const bgImageSrcURL = findURIfromSUT(sut);
      const seen = new Set();
      for (const [k, _v] of bgImageSrcURL.queryPairs) {
        if (seen.has(k)) {
          throw new Error(
            `duplicate keys for '${k}' found in query parameters`
          );
        } else {
          seen.add(k);
        }
      }
    });
  });

@sherwinski
Copy link
Contributor

We could also deduplicate any w and h in in imgixParams since we're just going to include a determined width and height right afterwards.

@ericdeansanchez
Copy link
Contributor

Yeah, ideally I'd like to set things up in a way where the duplication doesn't happen to begin with. Hmm...

ericdeansanchez added a commit that referenced this issue Apr 22, 2021
* build: add @imgix/js-core as a dependency

* feat: impl targetWidths in terms of ImgixClient.targetWidths()

* feat: impl buildSrcSet in terms of @imgix/js-core

* fix: test that issue #763 has been fixed

* refactor: remove dead code

* refactor: tidy up

* fix: no need to include w or h if building fluid-width srcset
imgix-git-robot pushed a commit that referenced this issue Apr 22, 2021
## [9.1.0](v9.0.4...v9.1.0) (2021-04-22)

### Features

* integrate @imgix/js-core into react-imgix ([#780](#780)) ([690e7b6](690e7b6)), closes [#763](#763)

 [skip ci]
imgix-git-robot pushed a commit that referenced this issue Apr 23, 2021
## [9.1.0](v9.0.4...v9.1.0) (2021-04-23)

### Features

* integrate @imgix/js-core into react-imgix ([#780](#780)) ([690e7b6](690e7b6)), closes [#763](#763)

 [skip ci]
@imgix-git-robot
Copy link

🎉 This issue has been resolved in version 9.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ericdeansanchez added a commit that referenced this issue Apr 23, 2021
* fix(BackgroundImpl): re-render only if a larger image is needed

* refactor: component should update if props or imgixParams change

* chore(release): 9.1.0

## [9.1.0](v9.0.4...v9.1.0) (2021-04-22)

### Features

* integrate @imgix/js-core into react-imgix ([#780](#780)) ([690e7b6](690e7b6)), closes [#763](#763)

 [skip ci]

Co-authored-by: imgix-git-robot <support@imgix.com>
imgix-git-robot pushed a commit that referenced this issue Apr 24, 2021
### [9.1.1](v9.1.0...v9.1.1) (2021-04-24)

### Bug Fixes

* re-render Background only if a larger image is needed ([#782](#782)) ([53bb104](53bb104)), closes [#763](#763)

 [skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants