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

Add RTL support #274

Merged
merged 3 commits into from
Oct 1, 2017
Merged

Add RTL support #274

merged 3 commits into from
Oct 1, 2017

Conversation

adi518
Copy link
Contributor

@adi518 adi518 commented Aug 2, 2017

Adds #39

@@ -796,7 +818,8 @@
searching: this.searching,
searchable: this.searchable,
unsearchable: !this.searchable,
loading: this.mutableLoading
loading: this.mutableLoading,
rtl: this.rtl
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this here. The data properties above it are all modified internally by the component. Since props are immutable, they are proxied to mutable values here.

Since vue-select has no reason to change rtl, we don't need to create a mutable data property for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't recall making it mutable.

Copy link
Owner

@sagalbot sagalbot Oct 1, 2017

Choose a reason for hiding this comment

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

You have it defined as both a prop, and a data key, both with the rtl key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really strange, you'd get an error from Vue. IIRC, I set it as prop and then added it to the dropdownClasses computed prop.

Copy link
Owner

Choose a reason for hiding this comment

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

Whoah! I've reviewed too many PRs today.. The way github presented the changes totally makes it look like it was in data, but yeah, it's in dropdownClasses().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it appeared like that for me as well. Kinda confusing, mysterious ways of the Github.

@sagalbot
Copy link
Owner

sagalbot commented Oct 1, 2017

This has been on the list a long time. Thank you!

@sagalbot sagalbot added this to the v2.3.0 milestone Oct 1, 2017
@sagalbot
Copy link
Owner

sagalbot commented Oct 1, 2017

RTL Support

@sagalbot sagalbot merged commit 2960029 into sagalbot:master Oct 1, 2017
@sagalbot
Copy link
Owner

sagalbot commented Oct 1, 2017

Changed the API a bit here. I liked your implementation because it's quick: <v-select rtl></v-select> is all you need, but to keep things familiar, I switched to <v-select dir="rtl"></v-select> since it's a documented attribute in the HTML spec. The prop is a string that defaults to auto.

@adi518
Copy link
Contributor Author

adi518 commented Oct 1, 2017

I assigned the dir attribute. I don't think you can shorthand rtl. It'd be nice though. A minor typo there, "rlt" instead of "rtl". :)

@sagalbot
Copy link
Owner

sagalbot commented Oct 1, 2017

Yep, any boolean props you want to set to true, you can shorthand. ex:

<v-select taggable></v-select>

Pretty handy.

Anyways, here's the final API for this:

<v-select dir="rtl"></v-select>
<v-select dir="ltr"></v-select>
<v-select dir="auto"></v-select> // default

I replaced the rtl Boolean prop with the dir String prop. It'll add the proper classes if you set dir to rtl, and it adds the dir="rtl" on the root div.

@adi518
Copy link
Contributor Author

adi518 commented Oct 10, 2017

Oh. Didn't know that one. Slick. Silly me. Didn't notice the auto option of dir attribute, as documented in MDN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants