Skip to content
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

Adds the SVGMarkerElement Interface #4990

Merged
merged 14 commits into from
May 20, 2021
Merged

Conversation

rachelandrew
Copy link
Collaborator

Adds SVGMarkerElement.

Spec: https://www.w3.org/TR/SVG2/painting.html#InterfaceSVGMarkerElement

Reviewer: @jpmedley

This was a fairly fiddly one, the SVG API docs are pretty inconsistent. There is no overview page I can find for Interfaces, and lots of missing bits.

@rachelandrew rachelandrew requested a review from jpmedley May 14, 2021 08:46
@rachelandrew rachelandrew requested a review from a team as a code owner May 14, 2021 08:46
@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2021

Preview URLs

Flaws

URL: /en-US/docs/Web/API/SVGMarkerElement
Title: SVGMarkerElement
on GitHub
Flaw count: 2

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist
    • /en-US/docs/Web/API/SVGElement/createSVGAngle does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/markerHeight
Title: SVGMarkerElement.markerHeight
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/orientType
Title: SVGMarkerElement.orientType
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/viewBox
Title: SVGMarkerElement.viewBox
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/setOrientToAngle
Title: SVGMarkerElement.setOrientToAngle()
on GitHub
Flaw count: 2

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist
    • /en-US/docs/Web/API/SVGSVGElement/createSVGAngle does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/markerUnits
Title: SVGMarkerElement.markerUnits
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/preserveAspectRatio
Title: SVGMarkerElement.preserveAspectRatio
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/refY
Title: SVGMarkerElement.refY
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/refX
Title: SVGMarkerElement.refX
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/setOrientToAuto
Title: SVGMarkerElement.setOrientToAuto()
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/markerWidth
Title: SVGMarkerElement.markerWidth
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

URL: /en-US/docs/Web/API/SVGMarkerElement/orientAngle
Title: SVGMarkerElement.orientAngle
on GitHub
Flaw count: 1

  • macros:
    • /en-us/docs/web/api/svgfittoviewbox (url: /en-US/docs/Web/API/SVGFitToViewBox) does not exist

External URLs

URL: /en-US/docs/Web/API/SVGMarkerElement
Title: SVGMarkerElement
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/markerHeight
Title: SVGMarkerElement.markerHeight
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/orientType
Title: SVGMarkerElement.orientType
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/viewBox
Title: SVGMarkerElement.viewBox
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/setOrientToAngle
Title: SVGMarkerElement.setOrientToAngle()
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/markerUnits
Title: SVGMarkerElement.markerUnits
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/preserveAspectRatio
Title: SVGMarkerElement.preserveAspectRatio
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/refY
Title: SVGMarkerElement.refY
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/refX
Title: SVGMarkerElement.refX
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/setOrientToAuto
Title: SVGMarkerElement.setOrientToAuto()
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/markerWidth
Title: SVGMarkerElement.markerWidth
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/SVGMarkerElement/orientAngle
Title: SVGMarkerElement.orientAngle
on GitHub

No new external URLs

(this comment was updated 2021-05-20 15:42:20.248260)

Copy link
Collaborator

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text looks pretty clean. Nice work.


<h2 id="Properties">Properties</h2>

<p><em>This interface also inherits properties from its parent, {{domxref("SVGElement")}}, and implements properties from {{domxref("SVGFitToViewBox")}}.</em></p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were documenting mixins in their targets. Or is that wishful thinking on my part. I know we've talked about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I meant to add a note about that. This is how it is documented through the rest of SVG, if we want to do otherwise then I should probably do a PR that copies the text to the others that use it. Let's discuss this next week too!

Copy link
Collaborator

@hamishwillee hamishwillee May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you ping me when there is resolution on this (and/or update appropriate howto docs)? Perhaps naïve, but I'd assumed that the approach for JavaScript mixins would be "global".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general we intended to merge them in, however I think we should also have consistency within sets of documentation. As this mixin was already documented elsewhere I just did what the rest of SVG did. If we don't want to do that then I'll document them here but also raise a PR to add the same text to the others.

Given #5057 I think the aim is to document them inline, so if Joe is happy for me to do it I can add the docs here, then sort the others in the same way (and remove the mixin).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would normally prefer being consistent within a documentation set. Give that we're trying to save the developers from needing to understand JS in terms of something not in JS, I'm ok with inconsistency in this case. I'm a fan of not letting perfect be the enemy of good.

Rachel, you are free to improve this as you see fit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the recent commit has added the two properties - viewBox and preserveAspectRatio.

Once we are happy with those, I will do a separate PR to add them to other pages that include this mixin.


<dl>
<dt>{{domxref("SVGMarkerElement.markerUnits")}}{{ReadOnlyInline}}</dt>
<dd>Returns an {{domxref("SVGAnimatedEnumeration")}} object, with one of the following values:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fourth comment I've written about this property. The IDL doesn't look like it works the way you say it does. (I'm not saying you're wrong. There's just something here I don't understand.) The meaning of SVGAnimatedEnumeration also seems to depend on which Object.property returns it, even though the constants reuse the same integers.

There's a lot of SVG to do, and I'll have to deal with this again. Let's meet so that you can explain this to me. I can't evaluate what I don't completely understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my code example was incorrect on this one, I just checked it again. It's the value of baseVal, unlike markerHeight etc. where you need to do baseVal.value

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added on everything the information about the value needing to be retrieved via baseVal.

files/en-us/web/api/svgmarkerelement/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/svgmarkerelement/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/svgmarkerelement/index.html Outdated Show resolved Hide resolved
---
<div>{{APIRef("SVG")}}</div>

<p class="summary">The <strong><code>orientType</code></strong> read-only property of the {{domxref("SVGMarkerElement")}} interface returns an {{domxref("SVGAnimatedEnumeration")}} object indicatign whether the {{SVGattr("orient")}} attribute is <code>auto</code>, an angle value, or something else.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p class="summary">The <strong><code>orientType</code></strong> read-only property of the {{domxref("SVGMarkerElement")}} interface returns an {{domxref("SVGAnimatedEnumeration")}} object indicatign whether the {{SVGattr("orient")}} attribute is <code>auto</code>, an angle value, or something else.</p>
<p class="summary">The <strong><code>orientType</code></strong> read-only property of the {{domxref("SVGMarkerElement")}} interface returns an {{domxref("SVGAnimatedEnumeration")}} object indicating whether the {{SVGattr("orient")}} attribute is <code>auto</code>, an angle value, or something else.</p>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on the 'something else'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec says "Some other value."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pinged Amelia as she may well know, as far as the spec goes (and in my testing) it looks like only auto-start-reverse which was added in SVG2 returns this, so I don't know why it says some other value. If you set it to some nonsense then it ends up as auto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reply from Amelia, which was kind of what I was assuming on this, but good to get a reply from someone more SVG knowledgeable. I've used this info to update this section, as Amelia points out it is pretty much impossible to get that value other than by using auto-start-reverse but the spec does leave it open as a possibility.

The auto-start-reverse value was new in SVG2, and for reasons I never quite understood, it was implemented without adding a new constant for the DOM property. There was a 2018 resolution to add in new enumeration values, but that never even got integrated in the spec, let alone in implementations: w3c/svgwg#424 (comment)

The Unknown value can indeed be used for unknown values, although mostly unknown values are discarded by the parser or getter/setter algorithms, so I'm not sure how you would make it so to test!

Anyway, current reality is that it means anything other than 'auto' or an explicit angle, including 'auto-start-reverse'.

files/en-us/web/api/svgmarkerelement/orienttype/index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very clean. Nice work!

@jpmedley jpmedley merged commit 5885c0d into mdn:main May 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants