draft: do not merge

[ericdeansanchez]

Here's a WIP, a start on a Background component that does not re-render infinitely (see #387).

Of course, this one has other issues (namely, it does not pass the browser testing portions of our test suite).

Before moving forward I'd like to get @frederickfogerty take on some of the initial design-goals/thought-process/testing-strategy/etc, e.g. what's the ideal behavior of this component supposed to be?

[commit-lint]

Draft

• do not merge (d23a952)

Contributors

ericdeansanchez

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

• @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
• @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
• @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
• @Commit-Lint merge disable will desactivate merge dependabot PR
• @Commit-Lint review will approve dependabot PR
• @Commit-Lint stop review will stop approve dependabot PR

[ericdeansanchez]

If you wanted to play around with this idea you can create a minimal react app and through this in App.js:

import "./App.css";
import { NewBackground } from "./NewBackground";

function App() {
  return (
    <NewBackground
      domain="assets.imgix.net"
      imgPath="examples/pione.jpg"
      maxWidth={1900}
      maxHeight={700}
    >
      <h2>Hello World!</h2>
    </NewBackground>
  );
}

export default App;

And throw this in src/NewBackground.js:

import React from "react";
import Measure from "react-measure";
import ImgixClient from "@imgix/js-core";

export class NewBackground extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      dimensions: {
        width: 500,
        height: 500,
      },
    };
  }

  shouldComponentUpdate(_nextProps, nextState) {
    return this.state.dimensions.width !== nextState.dimensions.width;
  }

  render() {
    const { width, height } = this.state.dimensions;

    const ixc = new ImgixClient({
      domain: this.props.domain,
      includeLibraryParam: false,
    });

    const imgURL = ixc.buildURL(this.props.imgPath, {
      w: width < this.props.maxWidth ? width : this.props.maxWidth,
      h: height < this.props.maxHeight ? height : this.props.maxHeight,
      fit: "crop",
    });

    const style = {
      /* Apply border-box to make widths inclusive of padding, etc. */
      boxSizing: "border-box",
      /* Apply 2px border for debugging. */
      border: "2px solid magenta",
      backgroundImage: `url(${imgURL})`,
    };

    return (
      <Measure
        bounds
        onResize={(contentRect) => {
          this.setState({
            dimensions: {
              // Only care about `width` and `height` for now
              width: contentRect.bounds.width,
              height: contentRect.bounds.height,
            },
          });
        }}
      >
        {({ measureRef }) => (
          <div height={height} width={width} ref={measureRef} style={style}>
            {width}, {height}
            {this.props.children}
          </div>
        )}
      </Measure>
    );
  }
}

Depending on where you're coming from, some issues with this code may be more apparent than others. One of the few bits of suspect lines of code is:

 backgroundImage: `url(${imgURL})`

This exists in the current Background component, but it seems suspicious to me... not sure why yet...

[sherwinski]

To shed some light on this part

One of the few bits of suspect lines of code is:
backgroundImage: `url(${imgURL})`

This is because the <Background> component is essentially a <div> that wraps content specified by the user; in this case the content being <h2>Hello World!</h2>. This displays the heading element against the background image of assets.imgix.net/examples/pione.jpg. The way we're able to add that image to to a <div> is by using the background-image CSS property, which is why it's a bit different from native image elements that we typically export.

[ericdeansanchez]

@sherwinski thanks for that! I should've been more clear about what I meant. Maybe what I was saying was something like, "I'm not completely aware of all/any edge cases using this CSS property introduces that we may or may not already be handling" if that makes sense.

[sherwinski]

@ericdeansanchez Ok sounds good, just wanted to make sure 👍

What you said did spark another idea I wanted to share: what if we prevent the component from requesting a smaller image whenever sizing down from the initial load. IIRC, browsers will just downsize a cached image rather than request a smaller version so we can save the user extra renders (both in the component and the imgix sense).

[frederickfogerty]

Hey, I've taken a brief look over the PR and it seems to be going alright. I also left some thoughts in Slack so they also apply here. I'll do a proper code review when this is at a working implementation, but I'll answer this question now:

Before moving forward I'd like to get @frederickfogerty take on some of the initial design-goals/thought-process/testing-strategy/etc, e.g. what's the ideal behavior of this component supposed to be?

Honestly I don't have much to say here other than to reimplement the original behaviour? Sorry for being a bit unhelpful but I can't think of anything important here.

[ericdeansanchez]

Closing in favor of #782