Removes variant format on mobile selection. #263

JPJPJPOPOP · 2018-01-10T06:53:26Z

Removes variant formatting for mobile because it doesn't show up anyways.

houndci-bot · 2018-01-10T06:53:36Z

assets/js/translator.js

    $('#dstLangSelect').val(curDstLang);
+    var currentDstLangSelect = $('#dstLangSelect option[value="' + $('#dstLangSelect').val() + '"]');
+    if(currentDstLangSelect.text().charCodeAt(0) === 160) {
+        currentDstLangSelect.text(currentDstLangSelect.text().substring(4)); /* sass-lint:disable-line no-magic-numbers */


No magic number: 4 no-magic-numbers

houndci-bot · 2018-01-10T06:53:36Z

assets/js/translator.js

+
    $('#dstLangSelect').val(curDstLang);
+    var currentDstLangSelect = $('#dstLangSelect option[value="' + $('#dstLangSelect').val() + '"]');
+    if(currentDstLangSelect.text().charCodeAt(0) === 160) {


No magic number: 160 no-magic-numbers

houndci-bot · 2018-01-10T06:53:36Z

assets/js/translator.js

    $('#srcLangSelect').val(curSrcLang);
+    var currentSrcLangSelect = $('#srcLangSelect option[value="' + $('#srcLangSelect').val() + '"]');
+    if(currentSrcLangSelect.text().charCodeAt(0) === 160) {
+        currentSrcLangSelect.text(currentSrcLangSelect.text().substring(4)); /* sass-lint:disable-line no-magic-numbers */


No magic number: 4 no-magic-numbers

This isn't robust. There's no reason a variant can't be en_GB.

@sushain97 I do not understand exactly what you mean by your statement. I tried adding in comments to see if it might clarify anything.

Also, why isn't /* sass-lint:disable-line no-magic-numbers */ working?

@sushain97 I do not understand exactly what you mean by your statement. I tried adding in comments to see if it might clarify anything.

Why does 4 work?

Also, why isn't /* sass-lint:disable-line no-magic-numbers */ working?

Why would it work?

@sushain97 4 works because there are 4  : '    '.

AH. My bad. Yeah, that should be commented.

@sushain97 I did, see the new changes :D

houndci-bot · 2018-01-10T06:53:36Z

assets/js/translator.js


    $('#srcLangSelect').val(curSrcLang);
+    var currentSrcLangSelect = $('#srcLangSelect option[value="' + $('#srcLangSelect').val() + '"]');
+    if(currentSrcLangSelect.text().charCodeAt(0) === 160) {


No magic number: 160 no-magic-numbers

I'm not sure why the build is passing despite this error. It should be fixed.

It says that it is a warning not an error:

/home/circleci/project/assets/js/translator.js 639:54 warning No magic number: 160 no-magic-numbers 641:73 warning No magic number: 4 no-magic-numbers 646:54 warning No magic number: 160 no-magic-numbers 647:73 warning No magic number: 4 no-magic-numbers ✖ 4 problems (0 errors, 4 warnings)

houndci-bot · 2018-01-11T01:53:27Z

assets/js/translator.js

    $('#dstLangSelect').val(curDstLang);
+    var currentDstLangSelect = $('#dstLangSelect option[value="' + $('#dstLangSelect').val() + '"]');
+    if(currentDstLangSelect.text().charCodeAt(0) === 160) { /* sass-lint:disable-line no-magic-numbers */
+        currentDstLangSelect.text(currentDstLangSelect.text().substring(4)); /* sass-lint:disable-line no-magic-numbers */


No magic number: 4 no-magic-numbers

houndci-bot · 2018-01-11T01:53:27Z

assets/js/translator.js

+
    $('#dstLangSelect').val(curDstLang);
+    var currentDstLangSelect = $('#dstLangSelect option[value="' + $('#dstLangSelect').val() + '"]');
+    if(currentDstLangSelect.text().charCodeAt(0) === 160) { /* sass-lint:disable-line no-magic-numbers */


No magic number: 160 no-magic-numbers

houndci-bot · 2018-01-11T01:53:27Z

assets/js/translator.js

+    // Check if language begins with &nbsp;
+    if(currentSrcLangSelect.text().charCodeAt(0) === 160) { /* sass-lint:disable-line no-magic-numbers */
+        // If it does, temporarily remove the four spaces
+        currentSrcLangSelect.text(currentSrcLangSelect.text().substring(4)); /* sass-lint:disable-line no-magic-numbers */


No magic number: 4 no-magic-numbers

houndci-bot · 2018-01-11T01:53:27Z

assets/js/translator.js

    $('#srcLangSelect').val(curSrcLang);
+    var currentSrcLangSelect = $('#srcLangSelect option[value="' + $('#srcLangSelect').val() + '"]');
+    // Check if language begins with &nbsp;
+    if(currentSrcLangSelect.text().charCodeAt(0) === 160) { /* sass-lint:disable-line no-magic-numbers */


No magic number: 160 no-magic-numbers

Wait, this is all silly. Go find the actual character for  , make it a constant and use it everywhere instead.

@sushain97 What do you mean by use it everywhere?

Don't use 160 or  . Use the character itself instead after you put it into a constant (or if it shows up fine, just use it directly).

sushain97 · 2018-01-11T04:08:50Z

assets/js/translator.js

+    // Check if language begins with &nbsp;
+    if(currentSrcLangSelect.text().charAt(0) === ' ') {
+        // If it does, temporarily remove the four spaces
+        currentSrcLangSelect.text(currentSrcLangSelect.text().substring(4)); // eslint-disable-line no-magic-numbers


Just replace the non-breaking spaces now. Also, safe to remove the comments; the code's intention is now clear :) Also, .startsWith( is nicer than the charAt.

houndci-bot · 2018-01-11T04:22:56Z

assets/js/translator.js

    $('#dstLangSelect').val(curDstLang);
+    var currentDstLangSelect = $('#dstLangSelect option[value="' + $('#dstLangSelect').val() + '"]');
+    if(currentDstLangSelect.text().startsWith(' ')) {
+        currentDstLangSelect.text(currentDstLangSelect.text().replace(/ /g,''));


Irregular whitespace not allowed no-irregular-whitespace
A space is required after ',' comma-spacing

houndci-bot · 2018-01-11T04:22:56Z

assets/js/translator.js

    $('#srcLangSelect').val(curSrcLang);
+    var currentSrcLangSelect = $('#srcLangSelect option[value="' + $('#srcLangSelect').val() + '"]');
+    if(currentSrcLangSelect.text().startsWith(' ')) {
+        currentSrcLangSelect.text(currentSrcLangSelect.text().replace(/ /g,''));


Irregular whitespace not allowed no-irregular-whitespace
A space is required after ',' comma-spacing

sushain97 · 2018-01-11T04:24:01Z

assets/js/translator.js

-    if(currentSrcLangSelect.text().charAt(0) === ' ') {
-        // If it does, temporarily remove the four spaces
-        currentSrcLangSelect.text(currentSrcLangSelect.text().substring(4)); // eslint-disable-line no-magic-numbers
+    if(currentSrcLangSelect.text().startsWith(' ')) {


There's no need to do this with one space here or below. Just use the four spaces.

sushain97

Everything is good until they become normal after you re-click on the select after focusing somewhere else:

JPJPJPOPOP · 2018-01-12T07:02:35Z

@sushain97 The variants only lose their indentations temporarily when they are selected. Selecting a different language will cause the indentation to reappear. Given my code, one way to fix this is to detect if the select dropdown is open and add in the indentations. I tried implementing this, but this ended up in some funky functionality. First, the code for detecting if the select dropdown is open is not clean and might have some cases that it fails. Also, adding the indentation apparently causes the dropdown to close.

Another way I tried was with optgroup. The only problem is that when I try adding in a blank label, it creates an empty space. Otherwise it works fine.

CSS properties like padding and margins don't work well with options.

I'm kinda stuck. Do you have any suggestions?

sushain97 · 2018-01-12T22:45:25Z

Yeah, optgroup isn't a good solution and padding/margin certainly won't work. Why can't you hook onto the select open event and fix up any variants to have the correct indent?

…

On Jan 12, 2018 1:02 AM, "Jonathan Pan" ***@***.***> wrote: @sushain97 <https://github.com/sushain97> The variants only lose their indentations temporarily when they are selected. Selecting a different language will cause the indentation to reappear. Given my code, one way to fix this is to detect if the select dropdown is open and add in the indentations. I tried implementing this, but this ended up in some funky functionality. Another way I tried was with optgroup. The only problem is that when I try adding in a blank label, it creates an empty space. Otherwise it works fine. CSS properties like padding and margins don't work well with options. I'm kinda stuck. Do you have any suggestions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#263 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEBEfl4Zku_xTzG2UWEfIOcknziSinvnks5tJwOMgaJpZM4RY4Yf> .

sushain97 · 2018-01-12T22:48:01Z

On second thought, I wonder if we should just eliminate this indentation entirely. Mobile Chrome on Android certainly doesn't show it. You could check other devices and if it doesn't really show up anywhere, we need to find another solution besides non-breaking spaces or if there isn't really a good one, undo the non-breaking spaces and declare this a wontfix.

…

On Jan 12, 2018 4:45 PM, "Sushain Cherivirala" ***@***.***> wrote: Yeah, optgroup isn't a good solution and padding/margin certainly won't work. Why can't you hook onto the select open event and fix up any variants to have the correct indent? On Jan 12, 2018 1:02 AM, "Jonathan Pan" ***@***.***> wrote: > @sushain97 <https://github.com/sushain97> The variants only lose their > indentations temporarily when they are selected. Selecting a different > language will cause the indentation to reappear. Given my code, one way to > fix this is to detect if the select dropdown is open and add in the > indentations. I tried implementing this, but this ended up in some funky > functionality. > > Another way I tried was with optgroup. The only problem is that when I > try adding in a blank label, it creates an empty space. Otherwise it works > fine. > > CSS properties like padding and margins don't work well with options. > > I'm kinda stuck. Do you have any suggestions? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#263 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEBEfl4Zku_xTzG2UWEfIOcknziSinvnks5tJwOMgaJpZM4RY4Yf> > . >

JPJPJPOPOP · 2018-01-13T01:56:32Z

@sushain97 For iPhone, they all look like this for Firefox, Chrome, and Safari:

All centered with no indentation.

I don't believe that there is any better way than using non-breaking spaces. Maybe we could try using dashes?
With optgroup, we could do something like this:

JPJPJPOPOP · 2018-01-13T02:32:01Z

@sushain97

Why can't you hook onto the select open event and fix up any variants to
have the correct indent?

Well first, I couldn't find any select open event. All of ones I found had to detect for multiple cases, i.e. mouse clicks, keypresses, and they had edge cases possibly.
Also, adding back the indent, which I was trying to do with .text(), actually causes the select drop-down to close automatically.

sushain97 · 2018-01-13T05:04:21Z

@sushain97 For iPhone, they all look like this for Firefox, Chrome, and Safari:

In this case, let's get rid of the indentation on Mobile?

houndci-bot reviewed Jan 10, 2018

View reviewed changes

houndci-bot reviewed Jan 11, 2018

View reviewed changes

sushain97 reviewed Jan 11, 2018

View reviewed changes

houndci-bot reviewed Jan 11, 2018

View reviewed changes

JPJPJPOPOP force-pushed the variantformat branch from 6468425 to 37f640b Compare January 11, 2018 04:23

sushain97 reviewed Jan 11, 2018

View reviewed changes

JPJPJPOPOP force-pushed the variantformat branch from 37f640b to 9c4229e Compare January 11, 2018 04:26

sushain97 requested changes Jan 12, 2018

View reviewed changes

Removed variant spacing for mobile.

e4ba7f2

JPJPJPOPOP force-pushed the variantformat branch from 9c4229e to e4ba7f2 Compare January 13, 2018 06:26

JPJPJPOPOP changed the title ~~Fixes variant format on mobile selection.~~ Removes variant format on mobile selection. Jan 13, 2018

Merge branch 'master' into variantformat

b9115b3

sushain97 approved these changes Jan 15, 2018

View reviewed changes

sushain97 merged commit 0781dd3 into apertium:master Jan 15, 2018

Removes variant format on mobile selection. #263

Removes variant format on mobile selection. #263

Uh oh!

Conversation

JPJPJPOPOP commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushain97 left a comment

Choose a reason for hiding this comment

Uh oh!

JPJPJPOPOP commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sushain97 commented Jan 12, 2018 via email

Uh oh!

sushain97 commented Jan 12, 2018 via email

Uh oh!

JPJPJPOPOP commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JPJPJPOPOP commented Jan 13, 2018

Uh oh!

sushain97 commented Jan 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

JPJPJPOPOP commented Jan 10, 2018 •

edited

Loading

sushain97 Jan 11, 2018 •

edited

Loading

sushain97 Jan 11, 2018 •

edited

Loading

JPJPJPOPOP commented Jan 12, 2018 •

edited

Loading

JPJPJPOPOP commented Jan 13, 2018 •

edited

Loading