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

Allow multiple aspect ratios #294

Merged

Conversation

mohammednasser-32
Copy link
Contributor

Fixes #230

Add support for validating multiple aspect ratios, as done for content types, like:

validates :avatar, aspect_ratio: [:square, :portrait, :is_16_9]

@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from 7cd2fb0 to 407c718 Compare November 13, 2024 19:50
@mohammednasser-32
Copy link
Contributor Author

Hey @Mth0158, the tests are failing for one case minitest (3.2, rails_7_1, mini_magick)
However it is passing locally when running BUNDLE_GEMFILE=gemfiles/rails_7_1.gemfile bundle exec rake test, using ruby 3.2 and changing metadata to use DEFAULT_IMAGE_PROCESSOR (mini_magick)
Do you know how can I reproduce the error to solve it?

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 14, 2024

Hi @mohammednasser-32,
I'll correct the test once I am finished refactoring the metadata.rb file, so no worries, on that point :)

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 14, 2024

FYI you are making breaking changes to the gem (deleting error messages), you should try to not make such breaking changes. Because when we release for change people will have to change their own messages to make the gem work.

You could for example keep the messages as they are for the :with option, but add a new one for the :in option (a bit like :in content_type). This will also be better in term of UX for users since they have a better understanding of the error they are getting :)

@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from 407c718 to 2001b38 Compare November 14, 2024 17:37
@mohammednasser-32
Copy link
Contributor Author

@Mth0158 yes right, sorry for the mess..I thought we need to make it similar to content_type (only content_type_invalid is there)
I pushed another approach

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 15, 2024

No worries, let me know when it's ready for reviewing :)

@mohammednasser-32
Copy link
Contributor Author

It is 😅 feel free to review it and let me know if further changes are needed..thanks!

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 24, 2024

Hi @mohammednasser-32,
I was quite busy last week, I'll probably have time to do your review this week, I'll let you know once it's reviewed :)

@mohammednasser-32
Copy link
Contributor Author

@Mth0158 just a friendly reminder whenever you have the time 🙏😄

Copy link
Collaborator

@Mth0158 Mth0158 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR,
Overall it is a nice work, however I think your code could be more readable in the validator, it's hard to follow what is happening, I think it's mostly due to your method naming. Maybe try to spend some time improving this? The rest is ok 👍

lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
test/validators/aspect_ratio_validator_test.rb Outdated Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
config/locales/da.yml Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
@mohammednasser-32
Copy link
Contributor Author

@Mth0158 Thank you for the review 🙏 looking at the code again and you are absolutely right 😅
did some refactoring and renaming here..please let me know if more changes are needed, thank you

@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from 6d40dba to 4acaa58 Compare December 13, 2024 19:13
@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from 72134bd to f647234 Compare December 13, 2024 19:53
@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from f647234 to 1631967 Compare December 13, 2024 19:58
@mohammednasser-32
Copy link
Contributor Author

Thanks again for the reveiw! added the changes and rebased

@Mth0158
Copy link
Collaborator

Mth0158 commented Dec 14, 2024

@mohammednasser-32 better adding new correction commits rather than rebasing the whole thing, it makes the review process quite difficult because I have to start over every time :/
Not rebasing would also not make the review comments "Outdated" which is easier to review :). Plus it does things like:

image

But no worries I'll review it like this, no need to change things now.

Copy link
Collaborator

@Mth0158 Mth0158 left a comment

Choose a reason for hiding this comment

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

Thanks @mohammednasser-32 for the revamp. But I think there is still room for improvement, I think after this one we are good to go to merge though ;)

lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
lib/active_storage_validations/aspect_ratio_validator.rb Outdated Show resolved Hide resolved
@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from f571f49 to 8d18e56 Compare December 14, 2024 19:13
@mohammednasser-32 mohammednasser-32 force-pushed the allow_multiple_aspect_ratios branch from 8d18e56 to 98cfe67 Compare December 14, 2024 19:21
@mohammednasser-32
Copy link
Contributor Author

Thank you again and sorry for how messy this turned 😅
appreciate your review and comments...changes applied

@Mth0158
Copy link
Collaborator

Mth0158 commented Dec 21, 2024

Hi @mohammednasser-32!

Thanks again for your hard work! The PR is a great addition to the gem ✌️

@Mth0158 Mth0158 merged commit 0010ecb into igorkasyanchuk:master Dec 21, 2024
22 checks passed
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.

Add support for multiple aspect ratios
2 participants