Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Generating message preview on the fly (#645, #754) #757

Merged
merged 1 commit into from
Feb 1, 2017
Merged

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Jan 24, 2017

In order to fix #645 we needed to change the way we do rendering, because it is almost impossible to translate the content that was pre-rendered before and saved to database.
This PR introduces the new way of rendering — I've removed :preview and :rendered-preview from the message model and made the process dynamic. And since the rendering became dynamic, we can easily translate almost everything every time we need.

Fixes #645, #754

@alwx alwx self-assigned this Jan 24, 2017
@alwx alwx changed the title WIP: Generating message preview on the fly Generating message preview on the fly Jan 24, 2017
@alwx alwx requested a review from rasom January 24, 2017 19:18
@alwx alwx added ready and removed in progress labels Jan 24, 2017
@alwx alwx changed the title Generating message preview on the fly Generating message preview on the fly (#645, #754) Jan 25, 2017
handler: function (params) {
return {
event: "request",
params: [params.amount],
request: {
command: "send",
contentCommand: "request",
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of specifying this information here we could get it from parameters here https://github.com/status-im/status-react/blob/8663bdfada6f1d7690a6fb0ab1a88236eaef0e34/src/status_im/chat/handlers/send_message.cljs#L286-L286
Command's name can be found in the same map as chat-id and command, the path is [:command-name :command :name].
It would be great to start using specs there, but that's not objective of this PR.

@alwx alwx force-pushed the feature/#645 branch 2 times, most recently from 223c046 to 6134bfd Compare January 26, 2017 13:27
@alwx
Copy link
Contributor Author

alwx commented Jan 26, 2017

@rasom done, no more contentCommand in js file

@alwx alwx requested a review from rasom January 26, 2017 13:29
Copy link
Contributor

@rasom rasom left a comment

Choose a reason for hiding this comment

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

That's how it looks on device#1:

on device#2:

on device#1 after reloading:

Both iOS, device#1 English, device#2 Russian

@alwx
Copy link
Contributor Author

alwx commented Jan 30, 2017

@rasom I've updated the code. Please, check it now.
Please also note, that we now perform a jail call, so if there is a problem with jail, there also may be problems with message content.

@alwx alwx requested a review from rasom January 30, 2017 14:25
@alwx alwx added ready and removed in progress labels Jan 30, 2017
@rasom
Copy link
Contributor

rasom commented Jan 30, 2017

One more thing here: amount is shown in different formats on the same device, but I suppose it should be unified for the specific localization

should we fix this too, @alwx, @jarradh ?

@0xc1c4da
Copy link
Contributor

yes it's supposed to be regionalised.... didn't we have an issue for this?

@rasom
Copy link
Contributor

rasom commented Jan 30, 2017

That's what #754 is about, thanks.

@rasom rasom added in progress and removed ready labels Jan 30, 2017
@alwx
Copy link
Contributor Author

alwx commented Jan 31, 2017

@rasom how can I reproduce this bug?

@rasom
Copy link
Contributor

rasom commented Jan 31, 2017

I was using two devices: the first Android with English and the second iOS with the Russian language. Then I just sent some ETH between them.

@alwx
Copy link
Contributor Author

alwx commented Jan 31, 2017

Phew, it was hard :(

  1. Our i18n library doesn't support a localization of numbers. There is a function called I18n.toNumber, but it doesn't automatically add correct delimiters and separators;
  2. The solution for Web is to use something like Number(100000).toLocaleString("ru-RU"), but it doesn't work in Safari and WKWebView because Safari doesn't support any arguments for toLocaleString (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString#Browser_compatibility).

The final solution works, but it is a bit hacky — I use toLocaleString() for a pre-defined number and extract delimiter and separator (status-im.i18n, line 78) to use it later.

@alwx alwx added ready and removed in progress labels Jan 31, 2017
@alwx alwx requested review from rasom and removed request for rasom February 1, 2017 07:51
@rasom
Copy link
Contributor

rasom commented Feb 1, 2017

@alwx, does this issue #779 mean that this will be fixed in another PR?

@alwx
Copy link
Contributor Author

alwx commented Feb 1, 2017

@rasom it means that the code for generating preview messages in chat list will be rewritten and moved to JS. So I decided not to pay too much attention to this problem here because it will be reimplemented anyway.

@rasom rasom merged commit 78e7057 into develop Feb 1, 2017
@rasom rasom deleted the feature/#645 branch February 1, 2017 14:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requests are sent localised
3 participants