-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Implemented RTL support for legends and tooltips #6460
Conversation
8827f47
to
6d87dcd
Compare
This answers the biggest issue in #5800. |
@danielgindi could you provide a fiddle of these changes showing how a chart would look with them included? |
I’d love to. Is there a url generated by the CI with a dist version to be included in a fiddle? |
I know we have a url for |
Some docs might help as well in understanding what the valid inputs for these options are |
Updated with preview in original PR post |
Inside the tooltip, both the title and the legend are still on the left |
Yes the preview configuration was wrong. Fixed :-) |
|
Nope. This is also demonstrated in the preview. Look closely at the legend label that contains both Hebrew and 'abc'. Then remove the
This happens with the code on |
Great work @danielgindi, I've tested your implementation, it was great, i hope code reviewers of this great project merge this pull request to the master ASAP. CC @benmccann |
There are other places where we render text that seem like they'd need to be updated as well: core.scale.js (title and labels), plugin.title.js, scale.radialLinear.js (labels) |
@benmccann Yeah I didn't get to them yet, did not have the time. As for chart titles I always just use HTML for that, much more configurable. To me it looks like a feature that should never have been implemented anyway... |
Another thing I was thinking is that there should probably be just one option. If it's an RTL language it should probably be set for the whole chart. I'm not sure you'd want to have to set it separately for the legends, tooltips, title, labels, etc. That's also why I was thinking it'd make sense to implement it in the other places |
Well that may sound weird, but when you're dealing with mixed languages this tends to be an amazing feature. Of course this could be solved in another way - by adding RLM/LRM unicode marks to very specifically crafted positions in the text. But most people for some reason don't know about those. |
@benmccann Also, for changing the text direction for the whole chart - the user simply needs to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a bunch of comments about minimization / coding style. me.width
vs me.minSize.width
might actually have an effect in some cases, other comments are cosmetic.
Your CI fails for some reason... 🤔 |
@danielgindi I've fixed the CI in #6493. This PR is passing now |
@danielgindi you'll need to rebase this PR. It's probably because of #6497. Sorry for the difficultly |
* Specify *rtl=true* to set legend/tooltip direction to rtl * Specify *textDirection='ltr'|'rtl'* to force text direction
Implemented RTL support for legends and tooltips
https://plnkr.co/edit/wkQMS1dJU9ZxScP0Odv9?p=preview