Skip to content

Supporting copywriters, more streamlined translation edit. #133

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

Open
wants to merge 11 commits into
base: v2
Choose a base branch
from

Conversation

tomitrescak
Copy link
Contributor

  1. Ability to show the key in the UI and sort by the key or translation
  2. Ability to specify HTML translation as {{ mf ‘key’ }} which automatically names the text as “<$key”>
    • in many scenarios programmer does not care about immediate translations. It is also not clear whether that text has been translated or corrected by copyrighter, so it is better to keep showing only placeholders. placing {{ mf ‘key’ ‘key’ }} seems redundant.
  3. Translations are now edited inline not in message boxes below.

@gadicc
Copy link
Owner

gadicc commented May 27, 2015

Hey, so happy to be getting PRs for v2 already! This looks like a superb addition. You caught me at quite a busy time but I'll make sure to give this the time it deserves by the end of the week. Much appreciated.

In order to be router agnostic and to provide better control over
translation routes I have extracted the route functionality and moved
subscriptions on template. I  have also created a new server route
which I am handling using the webapp package. This route handles the
download of all translations. I have also updated the README for the
project showing how to configure the app both for Flow router and Iron
Router.
@gadicc
Copy link
Owner

gadicc commented May 29, 2015

Hey @tomitrescak.

I saw you made a separate branch for the router stuff, but it looks like the commits landed up here too. But it's ok, we can discuss it all in one PR, since there is some related stuff.

In general I think the PR is great! And addresses a lot of issues that have come up in the past (router agnosticism, sort keys, keys without text, etc). I'm very happy to have some help in this area. We still have a bit of work to do though.

Router Agnosticism

Like I said, I think this is great!

One goal for v2 is to make things more "magic", and "just work" with minimal user effort. e.g. we automatically set the locale on moment, parsleyvalidator, if they're detected. So rather than having extra setup instructions, I'd prefer msgfmt:ui to have both iron-router and flow-router (and flow-layout) as weak dependencies, and then automatically setup the routes if they're detected for either as relevant. In the README we can mention that these are both officially supported, and give a clue on what the user should do if they're using anything else.

The patch unfortunately breaks a lot of the iron-router stuff... I couldn't test it in iron-router at all. A few examples, on /translate:

TypeError: Cannot set property 'strings' of null
    at null.<anonymous> (client.js:122)
Template.mfTransLang.onCreated(function () {
  this.data.strings = {}; // line 122
  this.data.orig = mfPkg.native;
  this.data.trans = Session.get("translationLanguage");
});

this.data will only exist if the template has a data context. but also, it's a problem to use this for our own purposes, because if the context (which is reactive) is ever replaced, it will wipe out our data. I guess you don't feel this with flow-router, which is so good at never re-rendering, but we'll have to avoid designs like this.

In /translate/:lang:

Exception in template helper: TypeError: Cannot read property 'indexOf' of undefined
    at Object.Template.mfTransLang.helpers.hasMoreRows (http://localhost:5555/packages/msgfmt_ui.js?ff4a323d01d52875fdcc4c748e48e8bb086658bf:601:22)

In general, unfortunately even with flow-router, I couldn't get anything working. I just get a blank page. There's a <div id="__flow-root"></div> in the body, but it's empty. I think there's still some missing step.

Blank keys

Love this too. But you need to please add a check to not replace duplicates. e.g.

mf('format_time', 'Time Formatting');  // anywhere, once
mf('format_time');                     // anywhere else, should use 'Time Formatting'

but:

 warn:  [msgfmt:extracts]  { format_time: "<format_time>" } in examples/examples.html:364 replaces DUP_KEY { format_time: "Time Formatting" } in examples/examples.html:35

Editor

Unfortunately I couldn't get this working but some comments from looking at the description and the code:

  • Show key and enable sorting by it, great!
  • Would love to get routeNameFromTemplate working again, is there a way to do this with flow-router?
  • Using enter/tab is a problem, as these are used all the time in message format strings. See e.g. http://messageformat.meteor.com/examples (a little lower down) and editing them in /translate.
  • For the same reason, inline editing in the table I can foresee being a problem. I'm not sure if there's a way to handle both options, but if we can't the big editing area is more important. This isn't just for messageformat strings, for paragraphs in general it's must easier to be able to see a large portion of both the source and target strings.

Testing

I've been very lax about this in the past, but you'll see in msgfmt:core I've really been trying to make ammends. I'd really love for future code to include tests against the changes... if it will help to put in some test scaffolding for msgfmt:ui, let me know!

General

That's it. As I said in general it's great to get PRs for v2 already. I can see you've put a lot of work into this and I'm very happy to have someone else already familiar with the project's internals. I hope my comments above don't detract from your motivation to contribute, we just need to make this package great for all it's users, who unfortunately have a variety of packages and needs.

@tomitrescak
Copy link
Contributor Author

Good points. Strange that the flow router does not work for you, all is good on. My side. Did you setup Te routes? I'll have to test with iron router properly and also test on clean apps. Will try to address your points soon. And I never heard about weak dependencies in meteor, and searching for them popped your name;)

@tomitrescak
Copy link
Contributor Author

@gadicc had a bit of time so I addressed some comments. I have added weak dependencies on both routers and added the code for their configuration. Currently there is no middleware for login checks configured, but that's a matter of adding two lines of code. I have created a configuration function, which allows to configure the flow router routes, layouts and templates and initialised configuration to defaul ones. I have also added two exemplary applications showing the functionality of iron router and flow router. All seems to work. What is still missing is:

  • Blank keys
  • Testing

@DSpeichert DSpeichert changed the title Supporting copy wrighters, more streamlined translation edit. Supporting copywriters, more streamlined translation edit. Jul 23, 2015
@gadicc gadicc mentioned this pull request Aug 20, 2015
@Maxhodges
Copy link
Contributor

and what is suppose to happen in this case

mf('format_time', 'Time Formatting');  
mf('format_time', 'Format Time');                    

who wins? whichever comes later (like CSS)?

Can we write something in the official docs (or in an unofficial wiki) about how duplicate keys are handled?

@Maxhodges
Copy link
Contributor

this probably belongs here
I feel clicking 'Back to Translation Summary' should save if the translation string in the input is dirty (unsaved). Otherwise you have to move to another item in the original string list before you can click 'Back to Translation Summary" safely.

@Maxhodges
Copy link
Contributor

are we sure about this?

mf('format_time', 'Time Formatting');  // anywhere, once
mf('format_time');                     // anywhere else, should use 'Time Formatting'

seems possibly very brittle. If I change the name of a view's folder and it's jump lower down in my list of views because of the name change, then many messageFormat strings may get trashed because of dependence on load order? I'm not actually sure what I think should happen regarding duplicate keys, but I'm not sure if this blank key inheritance scheme is the best idea.

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

Successfully merging this pull request may close these issues.

3 participants