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

Pagemacro: include rather than transclude EffectTiming properties #7644

Merged
merged 2 commits into from
Sep 1, 2021

Conversation

hamishwillee
Copy link
Collaborator

This is part of fixing #3196 - replaces a page inclusion macro of the EffectTiming dictionary properties into the two places it is used: Element.animate(), KeyframeEffect()

I would like to update further, perhaps as a post process. To help with that, answers to the following would be great.

  1. There is also KeyframeEffectReadOnly() that redirects to KeyframeEffect() and a AnimationEffectReadOnly which we don't document. I can't see in spec but they do appear in https://docs1.w3cub.com/haxe~javascript/html/animationeffectreadonly/ . Is it "valid" or are these things that no longer really exist?
  2. The EffectTiming dictionary should be removed altogether but I'm not quite sure how to deal with it:
    • The properties have sub-docs which I presume I can pull in most of. That would be painful but is doable.
    • What concerns me is that the thing has its own BCD https://developer.mozilla.org/en-US/docs/Web/API/EffectTiming#properties that is not included in the Element.animate() compatibility. I presume the action here is to update that with each of these options?

@hamishwillee hamishwillee requested a review from a team as a code owner August 6, 2021 06:26
@hamishwillee hamishwillee requested review from Elchi3 and removed request for a team August 6, 2021 06:26
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2021

Preview URLs

Flaws

Note! 2 documents with no flaws that don't need to be listed. 🎉

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

  • broken_links:
    • Is currently http:// but can become https://

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

  • macros:
    • /en-US/docs/Web/API/KeyframeEffectReadOnly/KeyframeEffectReadOnly redirects to /en-US/docs/Web/API/KeyframeEffect/KeyframeEffect
    • /en-US/docs/Web/API/KeyframeEffectReadOnly/KeyframeEffectReadOnly redirects to /en-US/docs/Web/API/KeyframeEffect/KeyframeEffect

External URLs

URL: /en-US/docs/Web/API/Element/animate
Title: Element.animate()
on GitHub

No new external URLs


URL: /en-US/docs/Web/API/KeyframeEffect/KeyframeEffect
Title: KeyframeEffect()
on GitHub

No new external URLs


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

No new external URLs


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

No new external URLs

(this comment was updated 2021-09-01 03:01:46.543388)

@sideshowbarker
Copy link
Collaborator

Not merging, because I don’t have answers to the questions in the issue description…

@hamishwillee
Copy link
Collaborator Author

Thanks!
@Elchi3 Can you help with the questions in the description? (or advise on who might)?

@teoli2003
Copy link
Contributor

To answer the 2nd question, I think EffectTiming may be one of these rare cases (the only other one I know being RTCConfiguration) where we want to have a separate page for a dictionary. They are more like interfaces without methods/properties (and unnamed for the Web dev).

@Ryuno-Ki
Copy link
Collaborator

By now, a wild merge conflict appeared.

@sideshowbarker
Copy link
Collaborator

I resolved the merge conflicts.

@teoli2003 teoli2003 merged commit ff7226b into mdn:main Sep 1, 2021
@hamishwillee hamishwillee deleted the pagemacro_animation branch September 1, 2021 07:33
@hamishwillee
Copy link
Collaborator Author

Thanks. I should have merged as is, but was still hoping for response from Florian. Better in than out. I'll think some more on this.
And cheers for your comment @teoli2003 !!!

@Elchi3
Copy link
Member

Elchi3 commented Sep 1, 2021

Sorry, I missed this in all the notifications. It looks good to me.

I think whether or not to consolidate EffectTiming under some constructors and not as a dictionary on its own is mostly best answered in BCD. Meaning that if we think we can make sensible structures there, the separate dictionary pages might not be needed. But this can be a follow-up. Thanks for getting rid of the page macro already.

mdn/browser-compat-data#7287 and mdn/browser-compat-data#6810 are related.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 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.

5 participants