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

Fix rtl issue take 2 #1159

Merged
merged 11 commits into from Apr 24, 2013
Merged

Fix rtl issue take 2 #1159

merged 11 commits into from Apr 24, 2013

Conversation

kenearley
Copy link

@pfiller This builds upon the idea in this PR #1096 (comment) except it's toggling a css class to do the positioning.

  • LTR select boxes will position left: -9000px
  • RTL select boxes will position left: 9000px
  • The scrollbar issue will still exist if you have both RTL and LTR on the page (fixed for demo page)
  • The scrollbar issue will still exist if the page isn't set in the same direction as you've set the select box (fixed for demo page)

cc: @stof, @ido-ran, @krembo99

@pfiller
Copy link
Contributor

pfiller commented Apr 24, 2013

@kenearley nice work. This made me realize we could simplify some code a bit by using a generic -with-drop class (currently, Chosen uses something like that only for the single version). I went ahead and made the change so scope it out.

@mlharvest there are some css changes here, but I don't think they're super impactful. Mind taking a quick gander?

This is good to go once you guys sign off.

@mlettini
Copy link
Contributor

Quick glance everything looks fine. Question, why is 9000px better/different than 9999px? personal preference?

@kenearley
Copy link
Author

why is 9000px better/different than 9999px? personal preference?

It just wasn't consistent. Makes it easier to do search if we stick to one number that represents 'move it way off the page'. I'm cool with whatever number.

@kenearley
Copy link
Author

I'm happy with this. :shipit:

@mlettini
Copy link
Contributor

Ok cool go with 9000. But in harvestapp/gh I usually use 9999 because its faster to hit four 9's and it's further you can go in four characters 🐎

@kenearley
Copy link
Author

@mlharvest Done!

@pfiller
Copy link
Contributor

pfiller commented Apr 24, 2013

I suck at merging.

kenearley pushed a commit that referenced this pull request Apr 24, 2013
This fixes the Right To Left scrollbar issue
@kenearley kenearley merged commit 37b64bc into master Apr 24, 2013
@kenearley kenearley deleted the fix-rtl-issue-take-2 branch April 24, 2013 18:19
@andriijas
Copy link

Why does it need to be moved off the screen rather than hidden?

@kenearley
Copy link
Author

@andriijas Good question. Here's an explanation given by @pfiller.

pfiller added a commit that referenced this pull request May 2, 2013
Fixes bug with Isolated Scrolling #1186
Remove unused get_side_border_padding #1169
Fix issue when using both jQuery & Prototype #1168
Fix choices_click method #1163
Isolate Chosen Scrolling #1155
Fix Right-to-Left scrollbar issue #1159
Add support for labels #1152
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.

None yet

4 participants