-
Notifications
You must be signed in to change notification settings - Fork 5.7k
feat(unstable): Geometry Interfaces Module Level 1 #27527
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
base: main
Are you sure you want to change the base?
Conversation
I'll work on using denoland/deno_core#1003 to reduce the JavaScript code. |
e7aa097
to
f3e416f
Compare
f3e416f
to
8102120
Compare
@bartlomieju After addressing changes denoland/deno_core#1187 and denoland/deno_core#1188 in deno_core with this PR, I believe this geometry API PR is ready for review and merge. Is it possible to include it in the next minor release? |
@petamoriken it will be hard because we want to do a release tomorrow. We can land it in one of patch releases afterwards (with an unstable flag or JSDocs stating the API is experimental) |
@bartlomieju I added |
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.
These are everything I have to say about the documentation. Haven't looked at the implementation or the tests yet.
* @experimental | ||
*/ | ||
declare var DOMMatrix: { | ||
prototype: DOMMatrix; |
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.
Maybe mark prototype
properties as readonly
?
prototype: DOMMatrix; | |
readonly prototype: DOMMatrix; |
Or maybe not, to stay consistent with lib.dom.d.ts
.
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've made it consistent with lib.dom.d.ts
.
&self, | ||
transform_list: &TransformList, | ||
) -> Result<(), GeometryError> { | ||
for transform in transform_list.0.iter() { |
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.
is2D
should be set to false
if the list contains any 3D transform function as defined by CSS, according to step 4 of parse a string into an abstract matrix. For example, new DOMMatrix("rotateZ(0)").is2D
should be false
. Though it seems that only Firefox implements this properly.
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.
Anyway, I've made it match the spec.
Co-authored-by: ud2 <sjx233@qq.com>
Co-authored-by: ud2 <sjx233@qq.com>
Co-authored-by: ud2 <sjx233@qq.com> Signed-off-by: Kenta Moriuchi <moriken@kimamass.com>
Co-authored-by: ud2 <sjx233@qq.com>
I'll follow the changes of #30827 tonight. |
I opened denoland/deno_core#1208 which will allow implementing |
closes #21608
blocked by:
cppgc_proto_object
issue deno_core#1127make_cppgc_empty_object
deno_core#1161name
andlength
properties toop2
object methods deno_core#1188