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

chore(macros/APIRef): separate static and instance members #7335

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

wbamberg
Copy link
Collaborator

@wbamberg wbamberg commented Oct 12, 2022

Summary

In the APIRef sidebar, have separate lists for static and instance properties and methods.

Problem

Currently the APIRef sidebar lists just "Properties" and "Methods", combining instance and static members and not distinguishing them.

Solution

This PR uses page types to build separate lists for instance and static members, as @Elchi3 suggested in #7223 (review).

This is in line with the recent work @hamishwillee has been doing in mdn/content#21353 and so on, and contributes to openwebdocs/project#104 .

This PR has a dependency on the l10 strings in https://github.com/mdn/content/blob/main/files/jsondata/L10n-Common.json. I've added strings for en-US in https://github.com/wbamberg/content/tree/l10-static-instance, so you can try this PR out. @SphinxKnight , if we want to go ahead with this can you notify the relevant people in the l10n community? Maybe they could add translations to that branch. I'm setting this PR to draft in the meantime.

  • the macro code change: 73db3e7 is super simple and clear
  • the unit test updates: ad26529 are a lot more complicated

Screenshots

Before

https://developer.mozilla.org/en-US/docs/Web/API/IDBKeyRange

Screen Shot 2022-10-12 at 9 40 26 AM

After

http://localhost:3000/en-US/docs/Web/API/IDBKeyRange

Screen Shot 2022-10-12 at 9 40 16 AM

How did you test this change?

@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Oct 12, 2022
@SphinxKnight
Copy link
Member

Thanks @wbamberg for the heads up :) @mdn/yari-content-ja / @mdn/yari-content-ko / @mdn/yari-content-es / @mdn/yari-content-pt-br / @mdn/yari-content-zh and @mdn/yari-content-fr please, if you can catch this one and add translations for your locale :)

@SphinxKnight
Copy link
Member

@wbamberg I opened wbamberg#1 for fr

@wbamberg
Copy link
Collaborator Author

@wbamberg I opened wbamberg#1 for fr

I'm not sure if you know this and are planning to come back to it, but your PR adds translations for the Yari unit test only. While that's fine, I don't think it matters to the substance of the tests to have real translations (all we need to test is that we fetch the correct string for the locale).

What we do need is translations for the strings in L10nCommon.json, in mdn/content, which is what the real macro will use. That would be a PR to https://github.com/wbamberg/content/tree/l10-static-instance .

@hamishwillee
Copy link
Contributor

@wbamberg Does it matter that your "after" list order does not match the docs heading order?

FWIW this is good to see.

@wbamberg
Copy link
Collaborator Author

@wbamberg Does it matter that your "after" list order does not match the docs heading order?

Oh, yeah, we should definitely have the same order. I'll push an update...

@SphinxKnight
Copy link
Member

SphinxKnight commented Oct 14, 2022

What we do need is translations for the strings in L10nCommon.json, in mdn/content, which is what the real macro will use. That would be a PR to https://github.com/wbamberg/content/tree/l10-static-instance .

I'll plug my brain into next time ^^ Thanks for the explanation

@wbamberg here it is wbamberg/content#261

@Graywolf9
Copy link
Contributor

Hi! Thank you so much for this! @wbamberg @SphinxKnight

I'm a little lost, for add/modifiy the es strings I do a new PR to https://github.com/wbamberg/content or into wbamberg/content#261

Thank you so much again

@SphinxKnight
Copy link
Member

Hi! Thank you so much for this! @wbamberg @SphinxKnight

I'm a little lost, for add/modifiy the es strings I do a new PR to https://github.com/wbamberg/content or into wbamberg/content#261

Thank you so much again

As Will wrote, a PR against https://github.com/wbamberg/content/tree/l10-static-instance

@wbamberg
Copy link
Collaborator Author

@Graywolf9 , I'm also happy to add the strings to that branch myself if you like, just let me know what to write. The only bit I can't do is the es translations :).

The relevant strings are at: https://github.com/wbamberg/content/blob/c7135af1cae9edd09e7840cce18c4b969c933eac/files/jsondata/L10n-Common.json#L32-L50 .

@Graywolf9
Copy link
Contributor

Thank you so much! @wbamberg , there are the strings

  "Static_methods": {
    "en-US": "Static methods",
    "es": "Métodos estáticos"
    "fr": "Méthodes statiques"
  },


  "Static_properties": {
    "en-US": "Static properties",
    "es": "Propiedades estáticas"
    "fr": "Propriétés statiques"
  },


  "Instance_methods": {
    "en-US": "Instance methods",
    "es": "Métodos de instancia"
    "fr": "Méthodes d'instance"
  },


  "Instance_properties": {
    "en-US": "Instance properties",
    "es": "Propiedades de instancia"
    "fr": "Propriétés d'instance"
  },

@wbamberg
Copy link
Collaborator Author

@wbamberg Does it matter that your "after" list order does not match the docs heading order?

Oh, yeah, we should definitely have the same order. I'll push an update...

I just pushed c33bd55, which ought to make the orders match up.

@wbamberg
Copy link
Collaborator Author

Thank you so much! @wbamberg , there are the strings

Thank you @Graywolf9 ! I just pushed your changes to my branch https://github.com/wbamberg/content/blob/daee8bb93ee5d7b90745388d9379d3bcddd1e130/files/jsondata/L10n-Common.json#L32-L54.

@yin1999
Copy link
Member

yin1999 commented Oct 15, 2022

Hi @wbamberg, I've created wbamberg/content#262. Is there anything else should I do?

@wbamberg
Copy link
Collaborator Author

Hi @wbamberg, I've created wbamberg/content#262. Is there anything else should I do?

That looks good to me, @yin1999 ! thank you!

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🎉

PS: I usually set myself a reminder to merge PRs like these 7 days after the l10n teams were pinged.

@caugner caugner changed the title Separate static and instance members in API sidebar chore(macros/APIRef): separate static and instance members Oct 20, 2022
@wbamberg wbamberg marked this pull request as ready for review October 20, 2022 19:37
@caugner caugner merged commit 5028478 into mdn:main Oct 20, 2022
@wbamberg
Copy link
Collaborator Author

wbamberg commented Oct 20, 2022

I just checked to see how this will look in various locales. fr, es, zh-CN, and ja. As we might hope, the first three display translated strings, and the last displays the en-US fallback:

fr

api-sidebar-fr

es

api-sidebar-es

zh-CN

api-sidebar-zh-cn

ja

api-sidebar-ja

@SphinxKnight
Copy link
Member

cc @mdn/yari-content-ja / @mfuji09 for strings on https://github.com/mdn/content/blob/main/files/jsondata/L10n-Common.json

@mfuji09
Copy link
Contributor

mfuji09 commented Oct 22, 2022

@SphinxKnight Thank you! I have made a PR for Japanese translation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants