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

change translate() parameter #48

Open
guillaumearm opened this issue Oct 12, 2016 · 9 comments
Open

change translate() parameter #48

guillaumearm opened this issue Oct 12, 2016 · 9 comments
Assignees
Milestone

Comments

@guillaumearm
Copy link
Collaborator

guillaumearm commented Oct 12, 2016

call translate(DummyComponent) should be forbidden.

should be call like this :

  • translate()(Dummy)
  • translate('')(Dummy)
  • translate('', Dummy)
@JalilArfaoui
Copy link
Contributor

As we discussed, I'd personally prefer that translate can be called either :

  • translate(MyComponent)
  • translate('some.polyglot.scope', MyComponent)
  • translate('some.polyglot.scope')(MyComponent)

Why would you want to forbid the first one ?

@guillaumearm
Copy link
Collaborator Author

because it's more simple to use and to remind.

  • i think it's better when arguments orders is always the same.
  • so, signature will be:
    translate :: polyglotScope -> Component -> TranslatedComponent
  • bonus: it is easier to migrate from react-polyglot

@JalilArfaoui
Copy link
Contributor

Mmm .. why not.
And do we still consider this usage :

translate({ polyglotScope: 'some.polyglot.scope' })(MyComponent)

or, in other words :

translate(options)(MyComponent)

?

It seems more complicated, but it enables us to support more option if the future, without changing signature.

@JalilArfaoui
Copy link
Contributor

One more thing ... maybe we could choose between

  • translate('scope', MyComponent)
  • translate('scope')(MyComponent)

If we want to maintain both, and keep using curry, to handle this call :

translate()(MyComponent)

We will have to test first arg type in translate to know if it's option or component.

This is Because of the way curry works :

const translate = curry(
  (options, component) => {
    console.log('options: '+options);
    console.log('component: '+component);
  }
);

translate()('component')('props');

output :

options: component
component: props

@guillaumearm
Copy link
Collaborator Author

guillaumearm commented Oct 20, 2016

let's go for :

  • translate({ defaultPolyglotScope: 'scope' })(MyComponent)
  • translate()(MyComponent)

@guillaumearm
Copy link
Collaborator Author

or translate({ polyglotScope: 'scope' })(MyComponent)
as you wish.

@satazor
Copy link
Contributor

satazor commented Dec 6, 2016

At the moment translate()(MyComponent) doesn't work which is quite strange as I use currying as it seems to be what the community is doing (need to do translate('')(MyComponent)).

Would you accept a PR to fix this use-case and land it on the current version? As I can see, the goal is later to move the react specific functionality to react-redux-polyglot correct?

@JalilArfaoui
Copy link
Contributor

Yes, it is a known problem.
We have to uncurry translate() so that it always returns a function.
Only drawback is that translate(MyComponent) won't work anymore but it won't in the future anyways so I guess it's not really a problem to do it now.
PR welcome :-)

As to the future of react-redux-polyglot package, we are discussing to have it as another npm module but in the same repo.

@guillaumearm guillaumearm added this to the v0.4 milestone Dec 6, 2016
@JalilArfaoui
Copy link
Contributor

I've started to refactor translate : https://github.com/Tiqa/redux-polyglot/tree/feat/multi_module_refacto
WIP, I'll continue tomorrow

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

No branches or pull requests

3 participants