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

added rtl support #1613

Merged
merged 5 commits into from
Oct 25, 2017
Merged

added rtl support #1613

merged 5 commits into from
Oct 25, 2017

Conversation

dekelb
Copy link
Contributor

@dekelb dekelb commented Mar 21, 2017

This PR is for RTL support.
Added examples for both single select and multi-select.

Merging this PR will close #1190, #1608

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.68% when pulling da6a5f7 on dekelb:feature/rtl_support into 93c3009 on JedWatson:master.

@JedWatson
Copy link
Owner

Thanks @dekelb!

Would you mind updating the .scss files as well? then I'm 👍 to merge this.

@dekelb
Copy link
Contributor Author

dekelb commented May 26, 2017

@JedWatson updated the .scss files as well. Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.883% when pulling 6031382 on dekelb:feature/rtl_support into 1d15068 on JedWatson:master.

Copy link
Collaborator

@agirton agirton left a comment

Choose a reason for hiding this comment

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

This looks great! Since there is a new prop can add documentation and unit tests to cover this?

@@ -1089,6 +1090,7 @@ const Select = createClass({
'is-pseudo-focused': this.state.isPseudoFocused,
'is-searchable': this.props.searchable,
'has-value': valueArray.length,
'Select-rtl': this.props.rtl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nit but it should probably be Select--rtl since rtl is a modifier. Also it feels like it should belong at the top just below the other modifiers. Can you update?

@sag1v
Copy link

sag1v commented Jun 5, 2017

Hi, nice work!
By inspecting the source code i noticed you need to pass a prop of rtl to the component. is there any examples for this? couldn't find it in the docs.

EDIT
@JedWatson @agirton just noticed this wasn't merged yet. any idea when will it get merged?

@agirton
Copy link
Collaborator

agirton commented Jun 5, 2017

Hi @sag1v this needs documentation and unit tests before we will merge this in.

@sag1v
Copy link

sag1v commented Jun 5, 2017

@agirton something i can help with? to speed things up... :)

@agirton
Copy link
Collaborator

agirton commented Jun 5, 2017

Let's give @dekelb one more week to see if he is able to take it on. If not feel free to jump in to finish.

@dekelb
Copy link
Contributor Author

dekelb commented Jun 6, 2017

@agirton a bit busy at work, but I'll do my best to add the docs, sample & tests. Regarding the tests - what did you have in mind regarding the test of this new prop?
@sag1v, you are welcome to fork my branch and PR on my code if you want.

@sag1v
Copy link

sag1v commented Jun 6, 2017

@dekelb regarding the tests i think you should have at least 1 that checks if the element has a .Select-rtl className when passing the rtl prop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 92.97% when pulling 7fca105 on dekelb:feature/rtl_support into 1d15068 on JedWatson:master.

@dekelb
Copy link
Contributor Author

dekelb commented Jun 10, 2017

Added a test on the rtl prop, and regarding the sample - the prev commit already has the rtl example (on the states and on the multiselect samples.

@sag1v
Copy link

sag1v commented Jun 11, 2017

I hope they will merge it soon, we need it as well :)

@dekelb
Copy link
Contributor Author

dekelb commented Jun 11, 2017

Hi Sagiv,
You are always welcome to use the branch in my repository until this PR is merged.
If you are using npm you can use:

"dependencies": {
    ...
    "react-select": "git://github.com/dekelb/react-select.git#7fca105"
    ...
}

@dekelb
Copy link
Contributor Author

dekelb commented Jun 22, 2017

@agirton any news regarding this PR? Thanks!

@gwyneplaine gwyneplaine merged commit 7fca105 into JedWatson:master Oct 25, 2017
gwyneplaine added a commit that referenced this pull request Oct 25, 2017
@gwyneplaine
Copy link
Collaborator

@dekelb thanks for this PR, its now been merged into master

@dekelb
Copy link
Contributor Author

dekelb commented Oct 25, 2017

@gwyneplaine thanks for the merge!

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.

Fix design for RTL
6 participants