Filters list do not preserve image ratio

[skjnldsv]

It seems the canva size is capped

filerobot-image-editor/packages/react-filerobot-image-editor/src/components/tools/Filters/FilterItem.jsx

Lines 13 to 14 in e1fb6f0

	const FILTER_PREVIEW_WIDTH = 60;
	const FILTER_PREVIEW_HEIGHT = 45;

Maybe it will be worth using a square canvas and containing the image within?

Or use the current image ratio. Might be a bit more tricky if the user edit the image size/crops the image

Context

Using latest filerobot-image-editor version@^4.3.1

[AhmeeedMostafa]

The upcoming release is considering a new behavior

[skjnldsv]

Awesome! Thank you very much @AhmeeedMostafa

[AhmeeedMostafa]

The issue is improved in version 4.3.8, please re-check it and let us know if all is good.

[skjnldsv]

@AhmeeedMostafa where is the commit that fixes this?

I am still experiencing the same issue

[AhmeeedMostafa]

@skjnldsv the issue is fixed, here u are a screenshot for a vertical image from the current demo page, please make sure that u are using the latest version (the new behavior is we are covering the filter item with the image keeping its ratio)

[skjnldsv]

@AhmeeedMostafa

filerobot-image-editor/packages/filerobot-image-editor/package.json

Line 37 in bf2b7bb

"react-filerobot-image-editor": "^4.3.7"

You released v4.3.8 for react-filerobot-image-editor, but filerobot-image-editor is not using it

[skjnldsv]

@AhmeeedMostafa maybe creating pull requests instead of pushing to master would allow other contributors like us to review and catch those mistakes? :)
We'd be happy to help in the future! 🤗 🙇

[skjnldsv]

Btw, on another note, can you explain the reasoning behind choosing a rendering ratio of 60x45 ?
Since you took the cropping approach, using a square would be more fitting from a UX perspective imho :)

See:

Google Photos	OneDrive

[AhmeeedMostafa]

@skjnldsv it is peer dependency not dependency, u should update it in ur project locally by installing the proper version for react-filerobot-image-editor the version u installed will be used, and for sure we welcome any PR that will be proper to merge.

for the filter size, It's a perspective from our UX team, but my POV it has more benefits than making it 45x45 as at least it will be useful for landscape to show the whole image if it's landscape and it is not good to increase the height more than that .

[skjnldsv]

@skjnldsv it is peer dependency not dependency, u should update it in ur project locally by installing the proper version for react-filerobot-image-editor the version u installed will be used, and for sure we welcome any PR that will be proper to merge.

Yes, but peer dependencies still show which version is being used for a specific package.
With npm 7 and above, you don't have to install peer dependencies. Npm will automatically do that and rely on the package peerDep to know which version to install.

Currently, if you only install filerobot-image-editor, npm will not fetch react-filerobot-image-editor@4.3.8, but 4.3.7
While the ^ is used and should imply that the patch version can change, npm does not seem to see it like that. (nor does dependabot)

I still think this should be addressed and fixed on release :)

[skjnldsv]

Closing as the cropping is working now! Thanks again for the hard work @AhmeeedMostafa ! 🎉

[AhmeeedMostafa]

@skjnldsv it is peer dependency not dependency, u should update it in ur project locally by installing the proper version for react-filerobot-image-editor the version u installed will be used, and for sure we welcome any PR that will be proper to merge.

Yes, but peer dependencies still show which version is being used for a specific package. With npm 7 and above, you don't have to install peer dependencies. Npm will automatically do that and rely on the package peerDep to know which version to install.

Currently, if you only install filerobot-image-editor, npm will not fetch react-filerobot-image-editor@4.3.8, but 4.3.7 While the ^ is used and should imply that the patch version can change, npm does not seem to see it like that. (nor does dependabot)

I still think this should be addressed and fixed on release :)

I'm aware that the new version of npm is doing that, but we are already mentioning in the docs the developer should install the react-filerobot-image-editor by himself, so if he did that by himself he will install the proper version, the peerDependency for defining the minimum required version to have functionalities work fine, last but not least I agree with the point of (^) that npm should care for it if it's going to install the peer dependency automatically.

so finally I see the peer dependency should be as it's currently & I don't think it's a bug , it's just about developer's implementation & notices. and let's see maybe we would have better idea/consideration for that in the future.

[skjnldsv]

Thank you for taking the time to clarify Ahmed :)