Dropdown Alignment for locale names - Fix #19, #127 #159

share-with-me · 2017-07-01T07:46:24Z

This PR aims to fix #19 and #127 . I make use of a bootstrap dropdown to display the locales. This dropdown is visible only on larger (non-mobile devices) and it reverts to the current select feature for displaying the locales while on mobile devices.

Currently, for 3 lanaguages, the bootstrap button does not display the name of the locale. For instance, qaraqalpaksha. I could not find this name (and 2 others) anywhere (neither in languages nor in iso639Codes and hence, am unable to populate this name currently ). How are the select options for these languages getting populated in the current functionality?

sushain97 · 2017-07-01T07:50:53Z

How are the select options for these languages getting populated in the current functionality?

Probably locales.json.

I make use of a bootstrap dropdown to display the locales.

I don't know how I feel about the styling of that dropdown. Screenshot?

share-with-me · 2017-07-01T07:52:07Z

I don't know how I feel about the styling of that dropdown. Screenshot?

Yep, just a second.

share-with-me · 2017-07-01T07:54:56Z

@sushain97 , After this PR, i would set up travis locally too. Please (pliss) pardon me on this one :P

share-with-me · 2017-07-01T07:58:54Z

Probably locales.json.

On it!

sushain97 · 2017-07-05T00:24:57Z

@share-with-me it looks a bit too much like a button now and not enough like a select looks. The bootstrap-select2 integration libraries might have some styles you can get inspiration from.

share-with-me · 2017-07-06T14:48:11Z

@sushain97 , I have updated the styling of the dropdown button. It looks like this now:

sushain97 · 2017-07-06T14:49:36Z

Does the alignment of “English” match that of a normal select? Seems to be center instead of right?

…

On Jul 6, 2017, at 10:48 AM, Monish Godhia ***@***.***> wrote: @sushain97 <https://github.com/sushain97> , I have updated the styling of the dropdown button. It looks like this now: <https://user-images.githubusercontent.com/8894636/27916939-2bbb5db0-6288-11e7-8d05-9f0ca0e21133.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEBEfreXwIVepA3CIo06iuvnSqZNW-qoks5sLPOrgaJpZM4OLP3q>.

share-with-me · 2017-07-06T14:58:52Z

Seems to be center instead of right?

Um, 'English' should appear in the left if that is what you are saying? Should it look something like below?:

sushain97 · 2017-07-06T17:33:10Z

Yes.

…

On Jul 6, 2017, at 10:58 AM, Monish Godhia ***@***.***> wrote: Seems to be center instead of right? Um, 'English' should appear in the left if that is what you are saying? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEBEftPGZYdgUZXg7UJB8i3BVMOyaujSks5sLPYsgaJpZM4OLP3q>.

share-with-me · 2017-07-06T17:55:11Z

Made the changes @sushain97 ! Made some changes in logic too after determining an edge case that was failing.

share-with-me · 2017-07-11T02:42:51Z

Added the appropriate paddings!

sushain97 · 2017-07-12T01:26:25Z

It looks good! Some tweaks requested.

Let's align both the left edges. Preferably making them the same width c.f. Firefox:

Also, let's use the blue from the other dropdowns:

Finally, the scrollbar overlaps with these options in Chrome:

Let's introduce a right margin?

sushain97

Some questions

sushain97 · 2017-07-12T01:27:14Z

assets/js/localization.js

@@ -115,6 +117,8 @@ $(document).ready(function () {
            window.history.replaceState({}, document.title, newURL);
        }
    }
+
+    $('#localeDropdownCaret').css('left', rtlLanguages.indexOf(locale) !== -1 ? '5%' : '90%');


What is this doing?

This is the positioning of the caret in the locale dropdown button. I am fixing up the position of localeName and the caret.

It is the position of the carets in the above 2 pics.

sushain97 · 2017-07-12T01:28:25Z

assets/js/localization.js

+                        .text(this[1])
+                        .prop('dir', rtlLanguages.indexOf(this[0]) !== -1 ? 'rtl' : 'ltr')
+                        .css('padding-left', rtlLanguages.indexOf(this[0]) !== -1 ? '115px' : '5px')
+                        .css('padding-right', rtlLanguages.indexOf(this[0]) !== -1 ? '5px' : '115px')


Hm, this seems a bit brittle?

Um, by brittle, do you mean that the logic to align the languages could be improved? Um, what suggestions do you have?

sushain97 · 2017-07-12T01:28:52Z

assets/js/localization.js

+            $('#localeDropdown').append(
+                $('<li></li>').append(
+                    $('<a>', {
+                        href: 'index.' + this[0] + '.html'


The current locale dropdown does not do this. We do not want to force a page refresh for switching languages! That's not great UX.

Hm yes. I would try to use the same logic and update this.

sushain97 · 2017-07-12T01:29:00Z

index.html.in

@@ -90,9 +90,11 @@
                      <p data-text="tagline" class="tagline"></p>
                  </div>
                  <div style="width: 35%" class="pull-right hidden-xs">
-                      <!-- <i class="icon-globe localeGlobe pull-right"></i> -->


share-with-me · 2017-07-13T18:41:19Z

@sushain97 , Made all the changes. Also implemented the functionality where the page is not forced to refresh now. I have emulated the existing functionality :)

sushain97

good work switching to the right behavior, some concerns

sushain97 · 2017-07-15T18:24:21Z

assets/js/localization.js

+            $('#localeDropdown').append(
+                $('<li></li>').append(
+                    $('<a>', {
+                        rel: this[0] // eslint-disable-line id-length


This doesn't seem to be a valid value for the rel attribute per https://www.w3.org/TR/html5/links.html#linkTypes. Have you tried using something like data-locale? Note that jQuery has a helpful $.data().

sushain97 · 2017-07-15T18:25:03Z

assets/js/localization.js

+                        .prop('dir', rtlLanguages.indexOf(this[0]) !== -1 ? 'rtl' : 'ltr')
+                        .css('padding-left', rtlLanguages.indexOf(this[0]) !== -1 ? '105px' : '5px')
+                        .css('padding-right', rtlLanguages.indexOf(this[0]) !== -1 ? '5px' : '105px')
+                        .hover(


This should go in CSS if at all possible. Is there a reason you put it here? CSS transitions might be helpful.

Hm, yep. It does make sense to put it in CSS. Would change it

sushain97 · 2017-07-15T18:28:10Z

assets/js/localization.js

+                        .text(this[1])
+                        .prop('dir', rtlLanguages.indexOf(this[0]) !== -1 ? 'rtl' : 'ltr')
+                        .css('padding-left', rtlLanguages.indexOf(this[0]) !== -1 ? '105px' : '5px')
+                        .css('padding-right', rtlLanguages.indexOf(this[0]) !== -1 ? '5px' : '105px')


Would it be possible to use text-align instead and then add padding on both sides?

sushain97 · 2017-07-15T18:28:59Z

assets/js/localization.js

+                        rel: this[0] // eslint-disable-line id-length
+                    })
+                        .text(this[1])
+                        .prop('dir', rtlLanguages.indexOf(this[0]) !== -1 ? 'rtl' : 'ltr')


I wonder if it would be worth not exporting rtlLanguages and instead a function like isRtlLanguage or something like that... I don't recall if we use rtlLanguages for any other purpose though.

Um, would making a function instead serve better? The entire usage of rtlLanguages is to determine if it is present in that particular array. Should I make a function instead?

Something like:

function isRtlLanguage(lang) { return rtlLanguages.indexOf(lang) !== -1; }

sushain97 · 2017-08-10T04:28:09Z

Um, but isn't scrollbar appearing 'always' a specific setting in OS? Are you suggesting that I could override it to always show?

I’m pretty certain it can also be done with CSS.

Also, if not, then when the user changes the settings of scrollbar, the appropriate margin gets added automatically right?

I don’t think it’s a margin per se. It’s out of the browser’s control.

…

On Aug 10, 2017, at 12:26 AM, Monish Godhia ***@***.***> wrote: Um, but isn't scrollbar appearing 'always' a specific setting in OS? Are you suggesting that I could override it to always show? Also, if not, then when the user changes the settings of scrollbar, the appropriate margin gets added automatically right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEBEfl2O6hjVllURbz94V65eJFnYbrf8ks5sWoZ7gaJpZM4OLP3q>.

share-with-me · 2017-08-10T14:19:23Z

@sushain97 , referring to this link, I have added the necessary CSS. Could you check how it looks now?

sushain97 · 2017-08-12T02:37:37Z

assets/css/style.css

@@ -338,6 +376,18 @@ body {
    text-decoration: none;
 }

+@media(device-width: 768px) and (device-height: 1024px){


sushain97

Small concern. I also still think we can get rid of localeToLanguageMap! I think you need to use $('.someElement').data('locale-name', 'blah').

sushain97 · 2017-08-19T03:38:37Z

assets/css/style.css

@@ -338,6 +376,18 @@ body {
    text-decoration: none;
 }

+@media(device-width: 768px) and (device-height: 1024px) {
+    ::-webkit-scrollbar {


Actually, this applies to all scrollbars on the page. We only want to affect the language dropdown one!

…eDropdown

share-with-me · 2017-08-19T14:30:52Z

Small concern. I also still think we can get rid of localeToLanguageMap! I think you need to use $('.someElement').data('locale-name', 'blah').

Thank you @sushain97 :D. I actually managed to achieve it.

share-with-me · 2017-08-19T14:38:13Z

Implemented both :D

sushain97 · 2017-08-19T21:59:55Z

assets/js/localization.js

+                $('<li></li>').append(
+                    $('<a>', {
+                        'data-locale': this[0]
+                    })


Can we use .data('locale', this[0]) for the sake of consistency?

sushain97 · 2017-08-19T22:00:36Z

assets/js/localization.js

        $.each(localePairs, function () {
+            var isRtlLanguage = (rtlLanguages.indexOf(this[0]) !== -1);
            $('.localeSelect').append(
                $('<option></option>')


I don't think that "</option>" is required here?

share-with-me · 2017-08-20T05:58:06Z

Made the changes.

sushain97 · 2017-08-20T15:27:22Z

assets/js/localization.js

+                        .data('locale', this[0])
+                        .text(this[1])
+                        .prop('dir', isRtlLanguage ? 'rtl' : 'ltr')
+                        .css({


Actually, on second thought, this should all go in the stylesheet. All you need to do is #localeDropdown li[dir='rtl'] { ... } .

I made the changes but doing a git rebase now is causing conflicts :/ .

share-with-me · 2017-08-20T18:58:25Z

Implemented the CSS changes.

jonorthwash · 2017-08-20T19:04:01Z

Screenshot?

share-with-me · 2017-08-20T19:06:31Z

jonorthwash · 2017-08-20T19:08:00Z

This looks good to me. I guess mostly we need it tested on OS X, though. @sushain97 ?

sushain97 · 2017-08-21T03:17:26Z

Hm, it appears good to me if I remove:

@media(device-width: 768px) and (device-height: 1024px) {

Why did you add this?

share-with-me · 2017-08-21T04:05:48Z

Why did you add this?

I had referred to some stackoverflow link and I was not sure how to test. I'd remove the media queries and update it now.

share-with-me · 2017-08-21T04:13:15Z

Removed the media queries @sushain97 .

share-with-me added 4 commits July 1, 2017 12:33

Language menu alignment

cb5801d

Remove trailing space

26813c0

Code style changes

f0ceb1b

sass-lint

a6a91f5

Travis

ac58ded

share-with-me added 2 commits July 1, 2017 15:13

Use locales.json to fetch current page name

0b5212a

Change ID name

f56e6cd

Styling the dropdown button

bda28d6

Travis

405ab95

Style and align like select

ee1f6b4

padding added to locale button names

d7a6cbe

sushain97 requested changes Jul 12, 2017

View reviewed changes

share-with-me added 3 commits July 13, 2017 23:58

Changes

3c12e62

Travis

ba1fbd9

sass-lint disable for !important

f2a5416

CHange cursor on hover of button to default

3525be4

sushain97 requested changes Jul 15, 2017

View reviewed changes

share-with-me added 2 commits August 10, 2017 19:37

Add CSS to force visible a scrollbar on OSX

388db58

Travis and rename pageLocales to localeToLanguageMap

bc98919

sushain97 reviewed Aug 12, 2017

View reviewed changes

Space added between paren and selector

cc01036

sushain97 requested changes Aug 19, 2017

View reviewed changes

Remove localeToLanguageMap and append this as key-value data to local…

f4e7e8d

…eDropdown

Style only the scrollbar corresponding to language dropdown

95abf26

sushain97 reviewed Aug 19, 2017

View reviewed changes

Use data method and remove closing option tag

8018e6a

Update localization.js

83124bf

sushain97 requested changes Aug 20, 2017

View reviewed changes

share-with-me added 3 commits August 20, 2017 21:28

Move styles to css file

da42d9c

Travis

a8cba41

Travis

67a3cb5

Remove media quey for dropdown scrollbar

449e813

Merge branch 'master' into gsoc-mono-language-menu

32163af

TinoDidriksen force-pushed the master branch from 0f5d286 to cf0ba05 Compare April 13, 2021 05:28

Dropdown Alignment for locale names - Fix #19, #127 #159

Are you sure you want to change the base?

Dropdown Alignment for locale names - Fix #19, #127 #159

Uh oh!

Conversation

share-with-me commented Jul 1, 2017

Uh oh!

sushain97 commented Jul 1, 2017

Uh oh!

share-with-me commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

share-with-me commented Jul 1, 2017

Uh oh!

share-with-me commented Jul 1, 2017

Uh oh!

sushain97 commented Jul 5, 2017

Uh oh!

share-with-me commented Jul 6, 2017

Uh oh!

sushain97 commented Jul 6, 2017 via email

Uh oh!

share-with-me commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sushain97 commented Jul 6, 2017 via email

Uh oh!

share-with-me commented Jul 6, 2017

Uh oh!

share-with-me commented Jul 11, 2017

Uh oh!

sushain97 commented Jul 12, 2017

Uh oh!

sushain97 left a comment

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!

share-with-me commented Jul 13, 2017

Uh oh!

sushain97 left a comment

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!

share-with-me Jul 16, 2017 • 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 commented Aug 10, 2017 via email

Uh oh!

share-with-me commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

share-with-me commented Jul 1, 2017 •

edited

Loading

share-with-me commented Jul 6, 2017 •

edited

Loading

share-with-me Jul 16, 2017 •

edited

Loading

sushain97 Aug 19, 2017 •

edited

Loading