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

[PE-4862] Updates background component to merge props #932

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

longevitytina
Copy link
Contributor

Description

This PR address issue: Background component doesn't utilize ImgixProvider's domain parameter

  • mergeComponentPropsHOF and processPropsHOF now wrap withContentRect("bounds")(BackgroundImpl)

BEFORE:
Imgix domain prop was not being passed to Background component from ImgixProvider

 <ImgixProvider domain={IMGIX_DOMAIN}>
      <Imgix
        src="image.jpg"
        width={100}
      /> {/* The Imgix component successfully uses the domain from the Provider */}
      <Background src="image.png" className="blog-title">
        <h2>Blog Title</h2>
      </Background> {/* The Background component fails to utilize the domain from the Provider */}
 </ImgixProvider>

AFTER:
Imgix domain prop can be access by Background component from ImgixProvider

How to test:

It takes a little set up to test, you'll need to add react-imgix to a repo and link to the working branch:

  1. create a react app yarn create vite my-react-app --template react
  2. CD into react-imgix and pull this branch, run npm run build
  3. CD into newly created my-react-app: yarn link react-imgix
  4. In my-react-app run: yarn dev
  5. Replace App.jsx with this code snippet:
import './App.css';
import Imgix, { Background, ImgixProvider } from 'react-imgix';

function App() {
  return (
    <>
      <ImgixProvider domain={IMGIX_DOMAIN}>
        <Imgix
          src='cats2.jpeg'
          width={500}
        />
        <Background
          src='lion.jpeg'
          className='blog-title'
        >
          <h2>Background Using ImgixProvider Domain</h2>
        </Background>
      </ImgixProvider>
    </>
  );
}

export default App;
  1. Expect to see all Images loading correctly on http://localhost:5173/
  2. Alternatively, see video of images, logging and changes:
sdk.mp4

@longevitytina
Copy link
Contributor Author

longevitytina commented Nov 30, 2023

tests are failing due to config file issues, trying to resolve with the following suggestions from, but no success yet:
CircleCI-Public/browser-tools-orb#72
CircleCI-Public/browser-tools-orb#75

Edit: this is a browser-tool orb issue that cannot be solved currently. CircleCI-Public/browser-tools-orb#98 (comment), so we'll ignore tests for now (locally they all pass)

UPDATE: This has been resolved by updating the node version in the config file

src/react-imgix-bg.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@luqven luqven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: great work on this! Nearly there.

comment: left one blocking question. Once that's answered/addressed we should be ready to merge.

test/unit/imgix-provider.test.jsx Show resolved Hide resolved
test/unit/imgix-provider.test.jsx Show resolved Hide resolved
test/unit/imgix-provider.test.jsx Outdated Show resolved Hide resolved
@longevitytina longevitytina force-pushed the PE-4862/bug-provider-domain branch 2 times, most recently from 90ad813 to e587057 Compare December 11, 2023 19:17
Copy link
Contributor

@luqven luqven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

Left one non-blocking suggestion.

test/unit/imgix-provider.test.jsx Outdated Show resolved Hide resolved
@longevitytina longevitytina merged commit 3134510 into main Dec 11, 2023
3 of 4 checks passed
@longevitytina longevitytina deleted the PE-4862/bug-provider-domain branch December 11, 2023 22:15
@imgix-git-robot
Copy link

🎉 This PR is included in version 9.8.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Background component doesn't utilize ImgixProvider's domain parameter
3 participants