Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Allow sending the number of lcd columns as a parameter #168

Merged
merged 3 commits into from
Dec 4, 2014

Conversation

amire80
Copy link
Contributor

@amire80 amire80 commented Dec 2, 2014

This is needed for https://phabricator.wikimedia.org/T76196
and may also be useful for many other features.

This is needed for https://phabricator.wikimedia.org/T76196
and may also nbe useful for many other features.
@@ -153,8 +153,7 @@
*/
renderRegions: function () {
var lcd = this, languages,
items = lcd.options.itemsPerColumn,
columns = 4;
items = lcd.options.itemsPerColumn;
Copy link
Member

Choose a reason for hiding this comment

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

Feels inconsistent to me to assign items but not columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll probably remove the items variable.

@Nikerabbit
Copy link
Member

Are you planning to do number of columns selection outside ULS? Wouldn't it be better inside ULS?

@santhoshtr
Copy link
Member

I used columns=1, menuWidth: 'narrow', I got
image

The column width is too small

@santhoshtr
Copy link
Member

That search icon need a litle bit of padding at left - Now it touch the border.
Column divs has three columns class. You need to make that dynamic. If class is wide, it is "three columns", If medium "six columns", if narrow "twelve columns". That should fix the above issue.

santhoshtr added a commit that referenced this pull request Dec 4, 2014
Allow sending the number of lcd columns as a parameter
@santhoshtr santhoshtr merged commit 08044f8 into wikimedia:master Dec 4, 2014
@amire80 amire80 deleted the columns branch May 31, 2017 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants