Replace Number type with a simpler Option<f32> #144
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
alice-i-cecile
added
usability
alice-i-cecile
commented
Jun 11, 2022
alice-i-cecile
commented
Jun 11, 2022
|
Tests are failing, likely due to my tweaks to the @mockersf has promised us a regenerated set of tests and associated bug fixes; I'd like to merge this change after that is in. |
alice-i-cecile
force-pushed
the
number-yeet
branch
from
1d48d1d
to
bf85308
Compare
mockersf
reviewed
Jun 11, 2022
Weibye
reviewed
Jun 11, 2022
This was referenced Jun 11, 2022
alice-i-cecile
force-pushed
the
number-yeet
branch
from
c046f96
to
05f0b2a
Compare
alice-i-cecile
commented
Jun 11, 2022
| ) -> Size<Number> { | ||
| Size { | ||
| width: node_size.width.or_else(parent_size.width - constants.margin.horizontal_axis_sum()) | ||
| - constants.padding_border.horizontal_axis_sum(), |
There was a problem hiding this comment.
The original implementation here was wrong; padding should only be subtracted if the node width is undefined.
There was a problem hiding this comment.
None of the tests seem to care either way about this change though 🙃
alice-i-cecile
added
breaking-change
jkelleyrtp
pushed a commit
that referenced
this pull request
Oct 10, 2022
* Update RELEASES.md * Implement extension traits for Option<f32> * Replace usages of Number with Option<f32> * Rename module from number.rs to math.rs * Note removal of associated traits * Make behavior of maybe_sub consistent * Thanks clippy! * dead_code XD * Update math operations to be consistent with previous impl * Reduce churn in PR * Follow line length calculation spec correctly * Fix undefined math logic for MaybeMath trait
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.
Objective
Numberis an unhelpful name, and had leaked into the public interface.This was actually just representing an
Option<f32>, and badly obscuring this fact.Fixes #83.
Feedback wanted
I particularly want feedback on the logic in the new-ish
MaybeMathtrait. This isn't entirely consistent with the behavior of the previous implementations onNumber, but those implementations were surprising in ways that seemed wrong.I can of course revert them to match exactly, but we should be sure we're following the spec (or at least Chrome's implementation of the spec).