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

Fix Background and fit property fixes #268 #270

Closed
wants to merge 1 commit into from

Conversation

verbeeksteven
Copy link

Fixes #268

@sherwinski
Copy link
Contributor

Hey @verbeeksteven, thanks for pointing this out and opening a PR. As you pointed out it looks like we are unintentionally hardcoding the fit parameter to crop, which should not be the case. I'd like to work on top of your changes to add some tests and do some general clean up for our linter before merging. I'll be sure to tag you if you'd like to review it beforehand as well.

@verbeeksteven
Copy link
Author

@sherwinski Yup! I wasn't sure if there was something else you wanted done. You are more then welcome to adjust/do what ever you want with my PR :)

@verbeeksteven
Copy link
Author

Any update @sherwinski I can make adjustments if you want.

@sherwinski
Copy link
Contributor

Hi @verbeeksteven thanks for following up and sorry for the hold up, the last week has been a bit busy.
If possible, could you please make adjustments to your PR by following this repo's pull request guide. More specifically, you will want to pay attention to the bug fix checklist. This will help us a lot with documentation and should fix any commit-lint issues we have before merging.
Thanks again for all your help so far!

@@ -88,7 +88,7 @@ const BackgroundImpl = props => {
...(disableLibraryParam ? {} : { ixlib: `react-${PACKAGE_VERSION}` }),
width,
height,
fit: "crop",
fit: (imgixParams.fit) ? imgixParams.fit : "crop",
Copy link
Contributor

Choose a reason for hiding this comment

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

If I could suggest tweaking things here for cleanliness:

const renderedSrc = (() => {
   const srcOptions = {
     fit: "crop",
     ...imgixParams,
     ...(disableLibraryParam ? {} : { ixlib: `react-${PACKAGE_VERSION}` }),
     width,
     height,
     dpr
   };

   return constructUrl(src, srcOptions);
 })();

@sherwinski
Copy link
Contributor

Hey @verbeeksteven sorry for the silence on this PR but don't worry, I did not forget about you! I am going to fix this issue as part of a separate PR, mostly to sidestep issues with prettier but also to add tests and whatnot. I'd love to add you as a reviewer if you'd like to take a look at the end result. Feel free to comment back with any questions.

@verbeeksteven
Copy link
Author

I haven't had a ton of time to do it either, I will certain test out any edits you do though!

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

Successfully merging this pull request may close these issues.

2 participants