-
Notifications
You must be signed in to change notification settings - Fork 10
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: allow to upload larger images without cropping any part of the image #102
Conversation
✅ Deploy Preview for cesium-pelo-mundo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
First of all, i really like the improvements that you made, but let's review a few things.
You can make that by adding a prop either to json and the function that parses the pictures. What's your thoughts on it? 👀 @AfonsoMartins26 Note: I didn't reviewed code in this, due to this improvements in the UI that are still needed to be done, after that is done i'll review it 🙌 |
It looks amazing @AfonsoMartins26 🙌 !! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me ✅ , but let the other reviewers @diogogmatos , @joaodiaslobo and @ruilopesm take a look into it !! 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I wonder if it would be possible to maybe discover the orientation of the image automatically through its width and height? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my initial pr, I implemented the feature. However, @MarioRodrigues10 said: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diogogmatos |
The screenshot was from the deploy preview on this PR... |
@diogogmatos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be fixed! Just make sure the checks pass ;)
Yes problem seems to be fixed @diogogmatos, I think @joaodiaslobo requested him to remove one or two lines and fix the CI, and after that it's good to go. 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a great addition! 🤩
No description provided.