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

feat(builder): add Builder.io #13

Merged
merged 10 commits into from
Feb 24, 2023
Merged

Conversation

steve8708
Copy link
Contributor

I think I did things right - lmk anything I got wrong :D

@ascorbic
Copy link
Owner

Thanks! There seems to be an issue with how resizing is handled though, which seems to be down to the Builder API. See this example image: https://cdn.builder.io/api/v1/image/assets%2FYJIGb4i01jvw0SRdL5Bt%2F462d29d57dda42cb9e26441501db535f?format=webp&width=500&height=600

The image is meant to be 500x600, but is actually 500x333. Resizing should follow the requested aspect ratio, not the aspect ratio of the original image. Is this an option with the API? Something like "cover" or "fill".

@steve8708
Copy link
Contributor Author

steve8708 commented Feb 23, 2023

Ah so for our API these are max dimensions and not forced dimensions. We specifically recommend using the width param only and setting an aspect ratio on your image (though we do support height too to set a maximum possible height). This can be tweaked though if important

@ascorbic
Copy link
Owner

ascorbic commented Feb 23, 2023

Aah, ok. This should actually be ok with unpic-img, because it uses object-fit: cover. The playground doesn't use the component though, so it gives the distorted appearance. The ideal would be if you had it as an option, like most CDNs do.

Co-authored-by: Matt Kane <m@mk.gg>
@ascorbic
Copy link
Owner

There are some test failures with the generated URLs. I'm not sure if the error is in the test or the transformer

@steve8708
Copy link
Contributor Author

steve8708 commented Feb 23, 2023

Dumb Q - how do I run the tests locally?

Also I realized I'm dumb - we do support width and height as expected, the default fit is 'inside', we also support fit='cover' which is the default behavior you expected. I can update this to be the default for what the image component sends, or I can just update the demo URL to use fit=cover too

@steve8708
Copy link
Contributor Author

right now I'm flying blind both with TS types not working locally and unclear how to run the tests - will use the CI for now to debug

@steve8708
Copy link
Contributor Author

Just updated to do the expected behavior (defaults to fit=cover) and should have TS and one test fixed, looking into the other test

@steve8708
Copy link
Contributor Author

OK I think I fixed everything - gotta hop into some mtgs but will check on the CI results when back and clear up any remaining issues

@ascorbic
Copy link
Owner

I need to document this! This is a Deno project, so it expects to use Deno for typechecking and running tests

@ascorbic
Copy link
Owner

So if you're uising VSCode, you install the Deno plugin and that should handle everything for you. You can run tests in VS Code, or with deno test src

@steve8708
Copy link
Contributor Author

Boom! working now, will have tests fixed in a bit 🙏

@ascorbic
Copy link
Owner

Awesome! Seems to mostly be just ordering of props.

@ascorbic
Copy link
Owner

I'm not sure why, but the URL in the playground isn;t working, though the tests are passing: https://deploy-preview-13--unpic-playground.netlify.app/

@steve8708
Copy link
Contributor Author

Ah yes, found the issue (the playground was not able to find the parser, but should now)

Also how do I test that out locally? I'm sure it's staring me in the face

display: grid;
place-items: center;
}

.imagePanel img {
object-fit: cover;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ascorbic I added this so that the img behavior matches the CDN ones

previously if you typed in a new value for width/height (esp one that the CDN needs to now generate) it would show a distorted image (stretch the image, which I think there is pretty much no case where people would want to do this in production - aka the default object-fit) and then it snaps into place when the CDN is updated

since all CDNs by default use cover, I changed the img behavior accordingly so when you change width/height it won't look distorted while the new one loads. in most cases you just see it be pixelated for a sec, and then it snaps to being nice and clear

felt more right/intuitive, but happy to remove if intentionally undesired

Copy link
Owner

Choose a reason for hiding this comment

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

I was going to change that myself, so thanks!

steve8708 added a commit to steve8708/unpic-img that referenced this pull request Feb 23, 2023
@ascorbic
Copy link
Owner

Great! To test locally, run npm run dev in the demo directory.

Copy link
Owner

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Great stuff! Thanks for the addition.

@ascorbic ascorbic changed the title Add Builder.io feat(builder): add Builder.io Feb 24, 2023
@ascorbic ascorbic merged commit fb31a94 into ascorbic:main Feb 24, 2023
This was referenced Feb 24, 2023
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.

None yet

2 participants