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

Translate /src/systems, support passing arguments to RichText slots #3340

Merged
merged 16 commits into from Dec 11, 2023

Conversation

pavish
Copy link
Member

@pavish pavish commented Dec 8, 2023

This PR:

  • Translates components within /src/systems.
  • Updates RichText component to support passing arguments to slots, enabling translation string syntax like:
    "click_for_info": "Click [linkComponent](here) for more information."
  • Removes a few slate components that are no longer used.

Notes for reviewers:

  • I've not translated errors that are never meant to occur in prod. These are exhaustive checks that are in place for making development easier.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@pavish pavish changed the title Translate /src/systems Translate /src/systems, support passing arguments to RichText slots Dec 10, 2023
@pavish pavish marked this pull request as ready for review December 10, 2023 19:32
@pavish pavish added the pr-status: review A PR awaiting review label Dec 10, 2023
@pavish pavish added this to the v0.1.5 milestone Dec 10, 2023
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

@pavish I'm reviewing the RichText component changes only.

I like the spirit of the changes to RichText. But I have some concerns with the syntax (which I recall expressing when you introduced this idea to me on a call).

The syntax [linkComponent](here) looks so much like a markdown hyperlink that I'm worried editors will mix up the order of linkComponent and here. I can see why you chose that order though — it's in keeping with putting the component name in the square brackets. I think on our call I had proposed reversing them with a syntax like [here](linkComponent), but I can see how that would also be confusing because the square brackets would be inconsistent with our existing RichText syntax — so I rescind that proposal.

What do you think about using one of these alternate syntaxes:

  • Click [linkComponent(here)] for more information.
  • Click [linkComponent:here] for more information.
  • Click [linkComponent here] for more information.

@seancolsen seancolsen assigned pavish and unassigned seancolsen Dec 11, 2023
@seancolsen seancolsen added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Dec 11, 2023
@pavish
Copy link
Member Author

pavish commented Dec 11, 2023

@seancolsen I thought a bit about this syntax.

The major concern I wanted to address is to ensure that translators know that anything within square brackets [linkComponent] should not be translated, however, everything within normal brackets (here) should be translated.

The alternative syntaxes you've provided would make it confusing to translators.

looks so much like a markdown hyperlink that I'm worried editors will mix up the order of linkComponent and here

I'm not concerned about this as much as these strings are only mentioned in a json file. I'm not sure if this is a concern we should address.

Having said that, I'm open to brainstorming a better syntax. I'd like to do that separately from this PR though.

Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

@pavish and I chatted on a call about this.

Our plan: We are going to merge this in as-is and then be sure to re-visit the syntax questions before we hire people to perform the translation work. We may decide to re-think the syntax within RichText component more broadly once we have a clearer idea of all the cases it needs to handle. At that point, if we make changes to the syntax, then we'll implement those syntax changes (modifying all the strings) before training any translators on it.

@seancolsen seancolsen added this pull request to the merge queue Dec 11, 2023
Merged via the queue into develop with commit f103ba7 Dec 11, 2023
19 checks passed
@seancolsen seancolsen deleted the translate-app-systems branch December 11, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: revision A PR awaiting follow-up work from its author after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants