Fix ratio calculation bug #1123
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 11 commits
March 19, 2019 16:22
…nto fix/logo-ratio-bug
adamszalapski
approved these changes
Mar 21, 2019
There was a problem hiding this comment.
Great work @danielamorse , from my perspective look perfect 👍
…s with decimals work as expected
sghoweri
approved these changes
Mar 22, 2019
There was a problem hiding this comment.
Nice job @danielamorse - this looks great!
As discussed, my only 2 cents on this is that I added two extra tests to make sure we're checking to make sure ratios that have fraction data contain decimals also works (just in case). Approved 👍
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira
Summary
Details
Previously, in
ratio.twigwe were dividing ratio height or width values by 100 when the value was > 100. This worked fine when both values were either > 100 or < 100 but failed when one was above and the other below. The error would miscalculate the aspect ratio, causing the visual regression we saw with the logo.Instead of dividing by 100, I'm now getting the greatest common divisor of our ratio values (via a new twig function). I added VR tests for logo and updated the tests to use the new reduced fractions.
How to test