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

Add resize scale enum variant #61

Closed
wants to merge 8 commits into from

Conversation

TadoTheMiner
Copy link
Contributor

This pr adds the Resize::Scale option in #59, and adds it to the demo instead of the placeholder. I've noticed that in the code three times static was used when const could be, so I replaced it. As I changed the demo to include the Resize::Scale option, the gif must be re-generated

@benjajaja
Copy link
Owner

First of all, thank you! I really had const and static mixed up, now I see that.

You can run an approximation of the CI with cargo make ci if you install cargo-make. Or just run clippy.

image

This is suprising to me, I would expect the image to scale down as well instead of being cropped. I also suspect it's not really getting cropped, just overdrawn, which might cause subtle problems down the line.

Could you look into this? I think for Scale(_), the "desired" area is not just the max(image, container) width and height, but instead the image scaled to always fit container and keeping the aspect ratio.

@TadoTheMiner
Copy link
Contributor Author

By the way do we really need to have text under the images in the demo?

@benjajaja
Copy link
Owner

It scales down, but sometimes it doesn't take up the full space if you play around with window size / layout.

image

By the way do we really need to have text under the images in the demo?

It helps a lot to eyeball glitches in different terminals. Since we're writing control sequences into some buffer, there can and have been conflicts with surrounding control sequences related to style.

@TadoTheMiner
Copy link
Contributor Author

image
This is an issue with the library in general as it occurs in Fit aswell. So I think you should open an issue about it, that both fit and scale don't resize properly

@benjajaja
Copy link
Owner

@TadoTheMiner okay, yea there seems to be some bug, although I can't exactly reproduce your screenshot.

I will merge this branch manually and push to master. I just disabled merge commits while you were working on this, sorry. I'll also add a note to the readme about this.

@benjajaja
Copy link
Owner

Rebased and merged in main. I'll have to check some general resize related bugs before I make a release.

@benjajaja benjajaja closed this Dec 9, 2024
@TadoTheMiner TadoTheMiner deleted the resize-scale branch December 9, 2024 09:52
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