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.

Sign up for GitHub

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

Jump to bottom

Remove obsolete Node.isSupported method #16996

Merged
merged 3 commits into from
Nov 19, 2023
Merged

Remove obsolete Node.isSupported method #16996

merged 3 commits into from
Nov 19, 2023

Conversation

leon-win
Copy link
Member

@leon-win leon-win commented Nov 16, 2023 •
edited
Loading

Description

This PR remove obsolete Node.isSupported method from MDN translated content.

Motivation

It has been removed from en-US locale a long time ago.

Related issues and pull requests

Relates to mdn/content#22863, #13499

@leon-win leon-win requested review from a team as code owners November 16, 2023 16:44
@leon-win leon-win requested review from JuanVqz, yanaklose, cw118, t7yang and josielrocha and removed request for a team November 16, 2023 16:44
@github-actions github-actions bot added l10n-fr Issues related to French content. l10n-zh Issues related to Chinese content. l10n-es Issues related to Spanish content. l10n-ru Issues related to Russian content. l10n-pt-br Issues related to Brazilian Portuguese labels Nov 16, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 16, 2023 •
edited
Loading

Preview URLs (8 pages)
Flaws (197)

URL: /zh-CN/docs/Web/API/Attr
Title: Attr
Flaw count: 19

  • macros:
    • /zh-CN/docs/Web/API/Attr/name does not exist but fell back to /en-US/docs/Web/API/Attr/name
    • wrong xref macro used (consider changing which macro you use)
    • wrong xref macro used (consider changing which macro you use)
    • wrong xref macro used (consider changing which macro you use)
    • /zh-CN/docs/Web/API/Attr/ownerElement does not exist but fell back to /en-US/docs/Web/API/Attr/ownerElement
    • and 12 more flaws omitted
  • broken_links:
    • Is currently http:// but can become https://
    • Is currently http:// but can become https://

URL: /fr/docs/Web/API/Attr
Title: Attr
Flaw count: 23

  • macros:
    • /fr/docs/Web/API/Attr/name does not exist but fell back to /en-US/docs/Web/API/Attr/name
    • wrong xref macro used (consider changing which macro you use)
    • wrong xref macro used (consider changing which macro you use)
    • wrong xref macro used (consider changing which macro you use)
    • /fr/docs/Web/API/Attr/ownerElement does not exist but fell back to /en-US/docs/Web/API/Attr/ownerElement
    • and 18 more flaws omitted

URL: /fr/docs/Web/API/Node
Title: Node
Flaw count: 24

  • macros:
    • /fr/docs/Web/API/Notation does not exist
    • /fr/docs/Web/API/Entity does not exist
    • /fr/docs/Web/API/EntityReference does not exist
    • wrong xref macro used (consider changing which macro you use)
    • /fr/docs/Web/API/Node/baseURIObject redirects to /fr/docs/Web/API/Node
    • and 18 more flaws omitted
  • broken_links:
    • Can't resolve /fr/docs/Web/JavaScript/Reference/Opérateurs/L_opérateur_this

URL: /ru/docs/Web/API/Attr
Title: Attr
Flaw count: 25

  • macros:
    • /ru/docs/Web/API/Element/getAttribute_ does not exist
    • /ru/docs/Web/API/Element/getAttributeNode_ does not exist
    • /ru/docs/Web/API/Attr/name does not exist but fell back to /en-US/docs/Web/API/Attr/name
    • /ru/docs/Web/API/Attr/namespaceURI does not exist but fell back to /en-US/docs/Web/API/Attr/namespaceURI
    • wrong xref macro used (consider changing which macro you use)
    • and 19 more flaws omitted
  • broken_links:
    • Anchor not lowercase

URL: /ru/docs/Web/API/Node
Title: Node
Flaw count: 28

  • macros:
    • /ru/docs/Web/API/ProcessingInstruction does not exist but fell back to /en-US/docs/Web/API/ProcessingInstruction
    • /ru/docs/Web/API/DocumentType does not exist but fell back to /en-US/docs/Web/API/DocumentType
    • /ru/docs/Web/API/Notation does not exist
    • /ru/docs/Web/API/Entity does not exist
    • /ru/docs/Web/API/EntityReference does not exist
    • and 20 more flaws omitted
  • broken_links:
    • Is currently http:// but can become https://
    • Can use the English (en-US) link as a fallback
    • Can use the English (en-US) link as a fallback

URL: /es/docs/Web/API/Attr
Title: Attr
Flaw count: 21

  • macros:
    • /es/docs/Web/API/Element/getAttributeNode does not exist but fell back to /en-US/docs/Web/API/Element/getAttributeNode
    • /es/docs/Web/API/Attr/name does not exist but fell back to /en-US/docs/Web/API/Attr/name
    • /es/docs/Web/API/Attr/namespaceURI does not exist but fell back to /en-US/docs/Web/API/Attr/namespaceURI
    • wrong xref macro used (consider changing which macro you use)
    • /es/docs/Web/API/Attr/localName does not exist but fell back to /en-US/docs/Web/API/Attr/localName
    • and 16 more flaws omitted

URL: /pt-BR/docs/Web/API/Attr
Title: Attr
Flaw count: 18

  • macros:
    • /pt-BR/docs/Web/API/Element/getAttributeNode does not exist but fell back to /en-US/docs/Web/API/Element/getAttributeNode
    • /pt-BR/docs/Web/API/Attr/name does not exist but fell back to /en-US/docs/Web/API/Attr/name
    • /pt-BR/docs/Web/API/Attr/namespaceURI does not exist but fell back to /en-US/docs/Web/API/Attr/namespaceURI
    • wrong xref macro used (consider changing which macro you use)
    • wrong xref macro used (consider changing which macro you use)
    • and 12 more flaws omitted
  • translation_differences:
    • Differences in the important macros (1 in common of 3 possible)

URL: /pt-BR/docs/Web/API/Node
Title: Node
Flaw count: 39

  • macros:
    • /pt-BR/docs/Web/API/Text does not exist but fell back to /en-US/docs/Web/API/Text
    • /pt-BR/docs/Web/API/Comment does not exist but fell back to /en-US/docs/Web/API/Comment
    • /pt-BR/docs/Web/API/CDATASection does not exist but fell back to /en-US/docs/Web/API/CDATASection
    • /pt-BR/docs/Web/API/DocumentType does not exist but fell back to /en-US/docs/Web/API/DocumentType
    • /pt-BR/docs/Web/API/Notation does not exist
    • and 31 more flaws omitted
  • broken_links:
    • Can use the English (en-US) link as a fallback
    • Can't resolve /pt-BR/docs/JavaScript/Reference/Global_Objects/Object
    • Can use the English (en-US) link as a fallback
External URLs (10)

URL: /zh-CN/docs/Web/API/Attr
Title: Attr

URL: /fr/docs/Web/API/Node
Title: Node

URL: /ru/docs/Web/API/Node
Title: Node

URL: /es/docs/Web/API/Attr
Title: Attr

URL: /pt-BR/docs/Web/API/Attr
Title: Attr

URL: /pt-BR/docs/Web/API/Node
Title: Node

(comment last updated: 2023-11-18 22:13:35)

@leon-win leon-win changed the title Remove obsolete Node.isSupported method Remove obsolete Node.isSupported method Nov 16, 2023
cw118
cw118 approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@cw118 cw118 left a comment

Choose a reason for hiding this comment

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

Looks good for l10n-fr, thank you @leon-win !

(It may be easier/faster to remove all obsolete methods in one locale in each PR, instead of removing one obsolete method for all locales, because only one reviewer would be needed for each PR.)

@leon-win
Copy link
Member Author

Looks good for l10n-fr, thank you @leon-win !

(It may be easier/faster to remove all obsolete methods in one locale in each PR, instead of removing one obsolete method for all locales, because only one reviewer would be needed for each PR.)

@cw118 thank you for reply!

I also wanted to do exactly that at first.
But then I thought that the changes are almost identical for all languages (might say trivial), and therefore one reviewer can look at them all :)
That is, in my opinion, the changes are understandable even without knowledge of a specific language. How do you think?

@cw118
Copy link
Member

cw118 commented Nov 16, 2023

I also wanted to do exactly that at first. But then I thought that the changes are almost identical for all languages (might say trivial), and therefore one reviewer can look at them all :) That is, in my opinion, the changes are understandable even without knowledge of a specific language. How do you think?

I agree that the changes are very small for all locales, but it happens somewhat often that locales have different conventions and requests, so even for this type of PR we prefer to wait for approval from a reviewer for each locale. Thanks for understanding!

@leon-win
Copy link
Member Author

I also wanted to do exactly that at first. But then I thought that the changes are almost identical for all languages (might say trivial), and therefore one reviewer can look at them all :) That is, in my opinion, the changes are understandable even without knowledge of a specific language. How do you think?

I agree that the changes are very small for all locales, but it happens somewhat often that locales have different conventions and requests, so even for this type of PR we prefer to wait for approval from a reviewer for each locale. Thanks for understanding!

I'm understood, thank you!
I actually have already created several more similar PRs 🥲
I am still convinced that bureaucracy can be unnecessary and can hinder progress.
In any case, the amount of time spent will be the same (if we wait for confirmation from all reviewers) -- it doesn’t matter one or several PR's =)

Graywolf9
Graywolf9 approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@Graywolf9 Graywolf9 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM for es

As @cw118 says, sometimes is better a PR per locale, so the locale can be updated soon, and then you can identify older locales that maybe ended hidden. Other times is good to have a unified change of all locales in a PR, we respect each locale lead so we wait to their review, depends on the change after two weeks someone more of the core team could approve the whole PR

Thank you so much for all your work and time!

yin1999
yin1999 approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

LGTM for zh-CN. Thank you @leon-win

@yin1999 yin1999 removed the request for review from t7yang November 17, 2023 00:46
saionaro
saionaro approved these changes Nov 17, 2023
View reviewed changes
Copy link
Member

@saionaro saionaro left a comment

Choose a reason for hiding this comment

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

LGTM for RU

@saionaro saionaro removed the l10n-ru Issues related to Russian content. label Nov 17, 2023
@yin1999 yin1999 removed the request for review from yanaklose November 17, 2023 09:21
@yin1999 yin1999 added the l10n-ru Issues related to Russian content. label Nov 17, 2023
@leon-win
Copy link
Member Author

LGTM for es

As @cw118 says, sometimes is better a PR per locale, so the locale can be updated soon, and then you can identify older locales that maybe ended hidden. Other times is good to have a unified change of all locales in a PR, we respect each locale lead so we wait to their review, depends on the change after two weeks someone more of the core team could approve the whole PR

Thank you so much for all your work and time!

Thanks for the explanation, @Graywolf9 🤗
I will keep it in mind in the future!

@leon-win leon-win mentioned this pull request Nov 19, 2023
Open
josielrocha
josielrocha approved these changes Nov 19, 2023
View reviewed changes
Copy link
Collaborator

@josielrocha josielrocha left a comment

Choose a reason for hiding this comment

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

Approve for pt-BR

@yin1999 yin1999 merged commit 0bb0afd into mdn:main Nov 19, 2023
7 checks passed
@leon-win leon-win deleted the fix/13499/remove-node-is-supported branch November 19, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josielrocha josielrocha josielrocha approved these changes

@JuanVqz JuanVqz JuanVqz approved these changes

@saionaro saionaro saionaro approved these changes

@Graywolf9 Graywolf9 Graywolf9 approved these changes

@yin1999 yin1999 yin1999 approved these changes

@cw118 cw118 cw118 approved these changes

Assignees
No one assigned
Labels
l10n-es Issues related to Spanish content. l10n-fr Issues related to French content. l10n-pt-br Issues related to Brazilian Portuguese l10n-ru Issues related to Russian content. l10n-zh Issues related to Chinese content.
Projects
l10n-fr
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@leon-win @cw118 @josielrocha @JuanVqz @saionaro @Graywolf9 @yin1999