kelly/header #105

kelly-binary · 2016-12-27T01:49:24Z

update language dropdown and header.

ashkanx · 2016-12-27T02:19:52Z

@kellybinary please fix the conflicts

aminmarashi · 2016-12-27T02:20:42Z

templates/partials/language.mustache

@@ -1,9 +1,9 @@
-	<span class="language-select">
+<!-- <span class="language-select">


@kellybinary Never keep the previous code in comments, git will take care of backing up codes.

aminmarashi · 2016-12-27T02:23:29Z

templates/bot.mustache

-                  </div>
-              </a>
-          </div>
+  <head>


@kellybinary
👍 for fixing the indentation.
It's always a good idea to pull the most recent changes from upstream before you make a PR. Because sometimes changes from other collaborators will cause merge conflicts.
If you don't have the upstream origin add it like this:

git remote add upstream https://github.com/binary-com/binary-bot.git

Then you will be able to perform git pull

git pull upstream master

Sometimes there'll be merge conflicts, you need to look thoroughly into the code to find out what has changed and keep the desired changes.
Test again to see if everything is as expected and then add the changes and commit them to complete the merge process.

aminmarashi · 2016-12-27T02:24:28Z

templates/bot.mustache

-              <th><span data-i18n-text="Buy Price"></span></th>
-              <th><span data-i18n-text="Final Price"></span></th>
-              <th><span data-i18n-text="Profit/Loss"></span></th>
+              <th data-i18n-text="No. of runs"></th>


@kellybinary 👍 for fixing the other parts of the code, better to make a separate commit for this fix.

@aminmarashi i didn't add this tho. it was already there, probably fetched from the beta branch

@kellybinary So better to revert. Let's keep the change set as concise as possible.

aminmarashi · 2016-12-27T02:32:58Z

src/common/translator.js

-  en,
-  ach,
-}
+    zh_tw: zhTw,


@kellybinary We use 2 spaces for each tab, not 4.

@aminmarashi noted. i will fix it.

aminmarashi · 2016-12-27T02:52:07Z

src/common/language_dropdown.js

@@ -0,0 +1,14 @@
+import $ from 'jquery'
+
+export default class LanguageDropdown {


🥇 for using ES6.
This function can be moved inside the translator constructor and be used instead of the event listener like this:

$('ul#select_language li') .click(function onClick() { const lang = $(this).attr('class') document.location.search = `l=${lang}` }) ....

In es6 encapsulation is the default behavior, therefore you don't need to define classes to ensure encapsulation. You could simply write:

export () => { $('ul#select_language li') .click(function onClick() { ... }

😄 you're right, needed to wrap it in another click event as such:

.click(function onClick() { $('ul#select_language li') .click(function onClick() { const lang = $(this).attr('class') document.location.search = `l=${lang}` }) })

aminmarashi · 2016-12-27T02:58:04Z

src/common/translator.js

    })
    const lang = this.getLanguage()
-    $('#language').val(lang)
+    $('.language').text(languageDisplayName[lang]);
+    $(document.getElementsByClassName(lang)[0]).hide();


shorter version thanks to es6:

$(`.${lang}`) .hide()

lovely shorter version!

aminmarashi · 2016-12-27T03:00:01Z

src/common/translator.js

+      document.location.search = `l=${value}`
+    })
+    $.each(languageDisplayName, (key, value) => { // populate language dropdown
+      $('#select_language').append(`<li class=${key}>${value}</li>`);


aminmarashi · 2016-12-27T03:01:38Z

src/common/translator.js

+    $('.language').on('change_language', (event, value) => {
+      document.location.search = `l=${value}`
+    })
+    $.each(languageDisplayName, (key, value) => { // populate language dropdown


Better not to use jquery each sth js is able to do:

for (const key of Object.keys(languageDisplayName)) { const value = languageDisplayName[key] ...

aminmarashi · 2016-12-27T03:18:32Z

@ashkanx Should we do the same for the ugly tour dropdown?

ashkanx · 2016-12-27T03:21:15Z

Yes. I think it's better to make it look like the other one.

kelly-binary · 2016-12-27T09:19:14Z

src/common/translator.js

-    $('#language').change(e => {
-      document.location.search = `l=${e.target.value}`
-    })
+    $('.language')


just read this comment, it looks good 🙋
…

On Dec 27, 2016 2:13 PM, "Amin Marashi" ***@***.***> wrote: In src/common/translator.js <#105 (comment)>: > export default class Translator { > constructor() { > - $('#language').change(e => { > - document.location.search = `l=${e.target.value}` > - }) > + $('.language') @kellybinary <https://github.com/kellybinary> How about this: const lang = this.getLanguage() $('#select_language li:first') .nextAll().click((e) => { const newLang = $(e.target).attr('class') document.location.search = `l=${newLang}` }) $('.language') .text($(`.${lang}`) .hide().text()) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/binary-com/binary-bot/pull/105/files/58f135dba88afa6fd3927d2b64ffc3f4f567df6a#r93899357>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXKLDhf9sdG7zGgTwhtF_g3v9t3ubItiks5rMKyIgaJpZM4LV-V8> .

coveralls · 2016-12-27T09:55:20Z

Coverage decreased (-0.09%) to 81.272% when pulling 6090d6f on kellybinary:master into 291f5db on binary-com:master.

kelly-binary · 2016-12-28T01:20:25Z

templates/bot.mustache

-              <th><span data-i18n-text="Buy Price"></span></th>
-              <th><span data-i18n-text="Final Price"></span></th>
-              <th><span data-i18n-text="Profit/Loss"></span></th>
+              <th data-i18n-text="No. of runs"></th>


okay, let's do the revert.
…

On Tue, Dec 27, 2016 at 6:03 PM, Amin Marashi ***@***.***> wrote: In templates/bot.mustache <#105 (comment)>: > <thead> > <tr> > - <th></th> > - <th></th> > - <th></th> > - <th></th> > - <th></th> > - <th></th> > - <th></th> > - <th></th> > + <th data-i18n-text="No. of runs"></th> @kellybinary <https://github.com/kellybinary> So better to revert. Let's keep the change set as concise as possible. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/binary-com/binary-bot/pull/105/files/a2327c037b07613fb5af045a9771c487b60cc3e8#r93892607>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXKLDjy9UlwIW1nY58Wbrb7vzuiyBaXwks5rMOJxgaJpZM4LV-V8> .

coveralls · 2016-12-28T02:09:58Z

Coverage decreased (-0.09%) to 81.272% when pulling 26d7baa on kellybinary:master into 291f5db on binary-com:master.

coveralls · 2016-12-28T02:26:31Z

Coverage decreased (-0.09%) to 81.272% when pulling d445940 on kellybinary:master into 291f5db on binary-com:master.

coveralls · 2016-12-28T02:36:22Z

Coverage decreased (-0.09%) to 81.272% when pulling b1d2423 on kellybinary:master into 291f5db on binary-com:master.

aminmarashi

All done, good job!

aminmarashi · 2016-12-27T03:02:57Z

src/common/translator.js

-}
+};
+
+const languageDisplayName = {


@kellybinary Is there any reason to do this in js? Why not simply define these in html?

Great! we no longer need this.

aminmarashi · 2016-12-27T03:05:12Z

templates/bot.mustache

-              <th><span data-i18n-text="Buy Price"></span></th>
-              <th><span data-i18n-text="Final Price"></span></th>
-              <th><span data-i18n-text="Profit/Loss"></span></th>
+              <th data-i18n-text="No. of runs"></th>


@kellybinary So better to revert. Let's keep the change set as concise as possible.

aminmarashi · 2016-12-27T03:29:59Z

.eslintignore

@@ -1,3 +1,5 @@
 **/mock/*.js
 **/translations/*.js
 **/calls.js
+src/common/language_dropdown.js
+src/common/translator.js


@kellybinary Not a good idea to ignore one of the main modules

@kellybinary Bump ^

aminmarashi · 2016-12-27T06:13:20Z

src/common/translator.js

-    $('#language').change(e => {
-      document.location.search = `l=${e.target.value}`
-    })
+    $('.language')


@kellybinary How about this:

const lang = this.getLanguage() $('#select_language li:not(:first)') .click((e) => { const newLang = $(e.target).attr('class') document.location.search = `l=${newLang}` }) $('.language') .text($(`.${lang}`) .hide().text())

aminmarashi · 2016-12-27T08:10:04Z

src/common/translator.js

@@ -30,14 +30,38 @@ const supportedLanguages = {
  fr,
  en,
  ach,
-}
+};


We don't use semicolons in Binary Bot js files. So let's keep them out of this! 😄

aminmarashi · 2016-12-27T10:14:41Z

templates/bot.mustache

@@ -41,7 +41,7 @@
 	</div>
  {{> loading }}
  <div id="topbar">
-	  <div class="right-header">
+	  <div class="wrapper">


Let's prefer right-header over wrapper. I will make the changes first thing tomorrow.

coveralls · 2016-12-28T02:38:38Z

Coverage decreased (-0.09%) to 81.272% when pulling 7a30497 on kellybinary:master into 291f5db on binary-com:master.

update language dropdown & header

a2327c0

aminmarashi mentioned this pull request Dec 27, 2016

Update header to match the new style #106

Closed

aminmarashi reviewed Dec 27, 2016

View reviewed changes

Merge remote-tracking branch 'upstream/master'

aef3af4

aminmarashi reviewed Dec 27, 2016

View reviewed changes

klyanyi added 3 commits December 27, 2016 10:40

fix indentation

3a7d38a

fix indentation

707fa20

remove indentation from template

f121b33

aminmarashi reviewed Dec 27, 2016

View reviewed changes

populate language dropdown in template

58f135d

kelly-binary reviewed Dec 27, 2016
View reviewed changes

klyanyi added 2 commits December 27, 2016 17:28

update language dropdown

ad6a566

Merge branch 'master' of github.com:binary-com/binary-bot

6090d6f

kelly-binary reviewed Dec 28, 2016
View reviewed changes

revert indentation

d445940

kelly-binary force-pushed the master branch from 26d7baa to d445940 Compare December 28, 2016 02:23

klyanyi added 2 commits December 28, 2016 10:31

remove src from eslintignore

b1d2423

hide untranslated languages from dropdown

7a30497

aminmarashi approved these changes Dec 28, 2016

View reviewed changes

aminmarashi merged commit 00d09c3 into binary-com:master Dec 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kelly/header #105

kelly/header #105

kelly-binary commented Dec 27, 2016

ashkanx commented Dec 27, 2016

aminmarashi Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016 •

edited

Loading

kelly-binary Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

kelly-binary Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

kelly-binary Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016

kelly-binary Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

kelly-binary Dec 27, 2016 •

edited

Loading

aminmarashi commented Dec 27, 2016

ashkanx commented Dec 27, 2016

kelly-binary Dec 27, 2016 via email

coveralls commented Dec 27, 2016

kelly-binary Dec 28, 2016 via email

coveralls commented Dec 28, 2016

coveralls commented Dec 28, 2016

coveralls commented Dec 28, 2016

aminmarashi left a comment

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

aminmarashi Dec 27, 2016

coveralls commented Dec 28, 2016

		@@ -1,9 +1,9 @@
		<span class="language-select">
		<!-- <span class="language-select">

		@@ -0,0 +1,14 @@
		import $ from 'jquery'

		export default class LanguageDropdown {

@@ @@ -30,14 +30,38 @@ const supportedLanguages = { @@
                 fr,
                 en,
                 ach,
-              }
+              };

kelly/header #105

kelly/header #105

Conversation

kelly-binary commented Dec 27, 2016

ashkanx commented Dec 27, 2016

aminmarashi Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

aminmarashi Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

aminmarashi Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

kelly-binary Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelly-binary Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelly-binary Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelly-binary Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

aminmarashi commented Dec 27, 2016

ashkanx commented Dec 27, 2016

kelly-binary Dec 27, 2016 via email

Choose a reason for hiding this comment

coveralls commented Dec 27, 2016

kelly-binary Dec 28, 2016 via email

Choose a reason for hiding this comment

coveralls commented Dec 28, 2016

coveralls commented Dec 28, 2016

coveralls commented Dec 28, 2016

aminmarashi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 28, 2016

aminmarashi Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016 •

edited

Loading

aminmarashi Dec 27, 2016 •

edited

Loading

kelly-binary Dec 27, 2016 •

edited

Loading

kelly-binary Dec 27, 2016 •

edited

Loading

kelly-binary Dec 27, 2016 •

edited

Loading

kelly-binary Dec 27, 2016 •

edited

Loading