-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Feature: rtl as initializer option #1491
Conversation
Looks good to me. Should we consider deprecating the classname? |
I vote for deprecating, as setting an option with a class is irregular. But that could create problems for current users of the |
We may have to leave both for a while and then remove the classname when we hit 2.0. We just need to update the docs to communicate this. We should probably update date the RTL portion of the docs in this PR to show the option use instead. |
Per SemVer:
If we introduce the constructor option but leave the class, we'd be bumping to 1.1 on release. If we remove the class, we'd be bumping to 2.0. Mostly a note for @pfiller and @kenearley for package-and-release purposes. |
@koenpunt I can take a stab at updating the docs, unless you're already on it. |
Be my guest :) |
I just noticed that the classname has precedence over the option. I think this should be reversed. Thoughts? |
This is because of backwards compatibility (if I remember it correctly). |
I don't see how that's backward compatible. If someone is using the classname then it will be rtl. It should fall through to checking the classname if the js option is not set. I'll check to make sure that is the case. |
Ah. Now I see. If you, for some reason, added the option |
Closing this now that we have a branch in the main repo. #1510 |
fixes #1485