-
Notifications
You must be signed in to change notification settings - Fork 4
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
Support for basic shape circle % #73
base: main
Are you sure you want to change the base?
Support for basic shape circle % #73
Conversation
22762b7
to
56ea25f
Compare
|
||
<div id="container"> | ||
<div id="target"> | ||
<img src="Nyancat.png" height="10%" width="10%"> |
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.
kill the cat
width: 100px; | ||
} | ||
#target { | ||
position: absolute; |
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.
Don't think we need this.
|
||
var aPositionValue = Number(aPositionValueString); | ||
if (aPositionUnit === '%') { | ||
if (Number(index) === 0) { |
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.
To get rid of magic numbers you could possibly set a boolean flag outside of the for loop. E.g var isWidth = true
// TODO: Need to support other positions as currently this only supports positions in which both x and y are specified and are in px | ||
if (analysedPosition.length < 2) { | ||
for (var i = analysedPosition.length; i < 2; i++) { | ||
if (Number(i) === 0) { |
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.
Could maybe use the bool flag idea here again
@@ -166,6 +262,7 @@ | |||
return undefined; | |||
} | |||
var toParse = [basicShapePolygonParse, basicShapeCircleParse, basicShapeInsetParse, basicShapeEllipseParse]; | |||
// var toParse = [basicShapeCircleParse]; |
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.
not needed anymore 😄
|
||
circlePathString = offsetPathParse('circle(10px at 50% 50%)', target).path; | ||
assert.equal(circlePathString, 'M 100 50 m 0,-10 a 10,10 0 0,1 10,10 a 10,10 0 1,1 -10,-10 z'); | ||
|
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.
Should add a test for garbage input to circle
if (radiusUnit[1] === '%') { | ||
var height = parentProperties.height; | ||
var width = parentProperties.width; | ||
if (!height || !width) { |
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.
I think this should just be checking !parentProperties.
|
||
<script> | ||
var keyframes = [ {offsetPath: 'circle(100px at 500px 500px)', offsetDistance: '0%'}, | ||
{offsetPath: 'circle(100px at 500px 500px)', offsetDistance: '100%'}]; |
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.
The demo should test percentages. I suspect it will fail given the dependency you have on the element during initial keyframe parsing.
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.
Although that issue is currently hidden by the soon to be removed startKeyframe == endKeyframe optimisation.
No description provided.