fix(target-size): update to match new spacing requirements #4117
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
straker
commented
Aug 2, 2023
| return { x, y: closestY }; | ||
| } | ||
| } | ||
| return Math.hypot(pointA.x - pointB.x, pointA.y - pointB.y); |
There was a problem hiding this comment.
This function exists in IE11 (I've verified)
| height: baseRect.bottom - baseRect.top, | ||
| width: baseRect.right - baseRect.left | ||
| }; | ||
| return new window.DOMRect( |
There was a problem hiding this comment.
I thought it was weird that split rects works on DOMRects but then doesn't return one, which meant that doing splitRect(rect1, rect2).left would return undefined instead of the x value.
WilcoFiers
requested changes
Aug 4, 2023
| `); | ||
| const nodeA = fixture.children[1]; | ||
| const nodeB = fixture.children[3]; | ||
| assert.closeTo(getOffset(nodeA, nodeB), 38, round); |
There was a problem hiding this comment.
Is the reason that this isn't exactly 40 because you're using buttons? This is why I used links when I wrote this originally, no min space, no padding, no border. I think you should use links so we get more consistent / predictable numbers.
There was a problem hiding this comment.
It's because the distance between the centers of both buttons gets subtracted by the circle radius (50 - 12).
WilcoFiers
previously requested changes
Aug 4, 2023
straker
commented
Aug 4, 2023
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
…-core into target-size-update
This was referenced Aug 7, 2023
WilcoFiers
approved these changes
Aug 8, 2023
Merged
1 task
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.
This mostly coverts the target-offset check to use the updated spacing exception from the spec. However, going over this we did notice that we still might be off in determining the center of the bounding rect is. The code currently only looks at the largest target rect but we are thinking we need to take into account the bounding box for all target rects and center on that. Will leave that as a tech debt ticket for after 4.8 so we can get this merged in on time.
Another tech debt ticket that needs to be created is to update the target-size check to use the new
getTargetSizefunction.Closes: #3891