Skip to content

Conversation

@MaathavanJkr
Copy link
Contributor

more sensible default language pair when apertium.org is loaded. I think it might work!

more sensible default language pair when apertium.org is loaded
Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for the PR.

No new JS should go into index.html.in. It should all go in assets/js.

@MaathavanJkr
Copy link
Contributor Author

@sushain97 how can i run this code?

@sushain97
Copy link
Member

What error did you run into when following the instructions in the README?

@MaathavanJkr
Copy link
Contributor Author

I ran it. :), But

@sushain97
Copy link
Member

But...?

@MaathavanJkr
Copy link
Contributor Author

okay im done i think. :)

@MaathavanJkr
Copy link
Contributor Author

Now the default translator pair is first available user's browser preference language pair. :) or it will select english as default. :)

@MaathavanJkr
Copy link
Contributor Author

https://maathavanjkr.github.io/ here you can see it in action. :)

@MaathavanJkr
Copy link
Contributor Author

#343

Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Thanks for moving this code!

Copy link
Member

@jonorthwash jonorthwash left a comment

Choose a reason for hiding this comment

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

This appears to change to the browser-preferred language even if it isn't an available for translation. This is incorrect behaviour.

@MaathavanJkr
Copy link
Contributor Author

This appears to change to the browser-preferred language even if it isn't an available for translation. This is incorrect behaviour.

Its Done I think. :) anymore changes? :)

jonorthwash
jonorthwash previously approved these changes Dec 12, 2019
Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Looking better! Still some concerns and broken tests apparently.

@sushain97
Copy link
Member

sushain97 commented Dec 17, 2019

I'm fine with

curSrcLang = iso639CodesInverse[lang];
autoSelectDstLang();

Just not happy with checking whether things exist in local storage first. If things do exist there, we should let restoreChoices('translator') which runs after us restore them and overwrite our changes. Does that not work?

@MaathavanJkr
Copy link
Contributor Author

I'm fine with

curSrcLang = iso639CodesInverse[lang];
autoSelectDstLang();

Just not checking whether things exist in local storage first. If things do exist there, we should let restoreChoices('translator') which runs after us restore them and overwrite our changes. Does that not work?

I think it will work. Lets try. :)

@MaathavanJkr
Copy link
Contributor Author

@sushain97 its working on both Chrome and IE. Can u please take a look at the code. :)

@sushain97
Copy link
Member

I'm not exactly sure we're on the same page. I suggested leaving this as is with your new code setting curSrcLang, etc. Then, restoreChoices('translator'); would run after setDefaultSrcLang() (as it already does), overwriting your settings.

@MaathavanJkr
Copy link
Contributor Author

MaathavanJkr commented Dec 17, 2019

I'm not exactly sure we're on the same page. I suggested leaving this as is with your new code setting curSrcLang, etc. Then, restoreChoices('translator'); would run after setDefaultSrcLang() (as it already does), overwriting your settings.

But it didnt work when running it after setDefaultSrcLang() function

@sushain97
Copy link
Member

Why?

@MaathavanJkr
Copy link
Contributor Author

Im not sure with it, wait i will have look.

@MaathavanJkr
Copy link
Contributor Author

MaathavanJkr commented Dec 17, 2019

Got it! Because restoreChoices('translator'); function doesnot set curSrcLang but changes the UI. So someanother function after it changes the UI with curSrcLang.

@sushain97
Copy link
Member

setCurSrcLang(storedCurSrcLang);
doesn't this do it?

@MaathavanJkr
Copy link
Contributor Author

setCurSrcLang(storedCurSrcLang);

doesn't this do it?

yeah, You are right.

I ran some tests and It seems the language we set to curSrcLang is set to Stored Lang when running function autoSelectDstLang();. then when we run restoreChoices('translator') its sets the same language. But i cant find where do that happens. Im trying...

@MaathavanJkr
Copy link
Contributor Author

Gotcha! In function autoSelectDstLang();, the function handleNewCurrentLang() is called. there the function persistChoices() is called which updates both curSrcLang and curDstLang to the local storage.
If its right we can simple solve it by calling handleNewCurrentLang() after restoreChoices('translator').

@sushain97
Copy link
Member

sushain97 commented Dec 19, 2019

Ah, jeez. Good detective work. I made some tweaks that I think should fix it?

Also, I wonder if we should be calling handleNewCurrentLang directly...

@MaathavanJkr
Copy link
Contributor Author

MaathavanJkr commented Dec 19, 2019

Ah, jeez. Good detective work. I made some tweaks that I think should fix it?

Also, I wonder if we should be calling handleNewCurrentLang...

Yeah!! I thought this to do inside set setDefaultLang();, But this one is better. Thanks! :)

@MaathavanJkr
Copy link
Contributor Author

MaathavanJkr commented Dec 19, 2019

Ah, jeez. Good detective work. I made some tweaks that I think should fix it?

Also, I wonder if we should be calling handleNewCurrentLang directly...

And i think we should call handleNewCurrentLang for curSrcLang under setDefaultLang(). as when im testing, the UI isnt set when we ran for first visit.

@sushain97
Copy link
Member

And i think we should call handleNewCurrentLang for curSrcLang under setDefaultLang(). as when im testing, the UI isnt set when we ran for first visit.

Yep, that sounds like the right move after taking a quick look at the code. Could you make that tweak and test it?

@MaathavanJkr
Copy link
Contributor Author

Yep, that sounds like the right move after taking a quick look at the code. Could you make that tweak and test it?

Done. :), Tested on both Chrome and IE for both instances and its working perfectly.

@sushain97
Copy link
Member

Seems good!

@sushain97
Copy link
Member

@jonorthwash This looks good on my end. Wanna try it before I merge? I don't have any particularly unique browser settings.

@sushain97 sushain97 merged commit 4bc6b47 into apertium:master Dec 24, 2019
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