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

Implemented RTL support for legends and tooltips #6460

Merged
merged 5 commits into from Sep 11, 2019

Conversation

danielgindi
Copy link
Contributor

@danielgindi danielgindi commented Aug 12, 2019

  • Specify rtl=true to set legend/tooltip direction to rtl
  • Specify textDirection='ltr'|'rtl' to force text direction

https://plnkr.co/edit/wkQMS1dJU9ZxScP0Odv9?p=preview

@danielgindi danielgindi force-pushed the feature/rtl branch 2 times, most recently from 8827f47 to 6d87dcd Compare August 12, 2019 15:39
@danielgindi
Copy link
Contributor Author

This answers the biggest issue in #5800.
As in many cases the data is numerical and we do want to present it left-to-right axis, but the texts, legends and tooltips should be RTL.

@etimberg
Copy link
Member

@danielgindi could you provide a fiddle of these changes showing how a chart would look with them included?

@danielgindi
Copy link
Contributor Author

I’d love to. Is there a url generated by the CI with a dist version to be included in a fiddle?

@benmccann
Copy link
Contributor

I know we have a url for master, but for PRs I've always just built the file locally and uploaded it. I liked Plunker for that the last time I did so

@benmccann
Copy link
Contributor

Some docs might help as well in understanding what the valid inputs for these options are

@danielgindi
Copy link
Contributor Author

Updated with preview in original PR post

@jjamid
Copy link

jjamid commented Aug 14, 2019

Updated with preview in original PR post

Inside the tooltip, both the title and the legend are still on the left

@danielgindi
Copy link
Contributor Author

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 :-)

@jjamid
Copy link

jjamid commented Aug 14, 2019

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 :-)

  1. It seems that the textDirection has no effect at all (no matter its value).
    Setting rtl to true changes the text direction (which I think is expected anyway).
    Am I right ?

  2. I noticed that the squares in the legend in line chart with fill=false has lost their background color and now have only a border.

elements: {
    line: {
	fill: false
    }
},

@danielgindi
Copy link
Contributor Author

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 :-)

  1. It seems that the textDirection has no effect at all (no matter its value).
    Setting rtl to true changes the text direction (which I think is expected anyway).
    Am I right ?

Nope. rtl=true makes the rendering direction rtl. pointStyle on the right, padding, text on the left.
textDirection='rtl' makes the text itself rtl. Which means that mixed that that contain both RTL scripts and LTR scripts - will be rendered correctly.
The following text:
עברית 123 English
Is rendered LTR, but in Hebrew it should actually show the Hebrew on the right, then the number, then the English part, like this:
English עברית 123
(This is rendered correctly only because I messed up the text to be in incorrect order).

This is also demonstrated in the preview. Look closely at the legend label that contains both Hebrew and 'abc'. Then remove the textDirection option.

  1. I noticed that the squares in the legend in line chart with fill=false has lost their background color and now have only a border.
elements: {
    line: {
	fill: false
    }
},

This happens with the code on master too. Not me :-D

@abouolia
Copy link

abouolia commented Aug 22, 2019

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

@etimberg etimberg requested review from kurkle and nagix August 24, 2019 18:30
@benmccann
Copy link
Contributor

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)

@danielgindi
Copy link
Contributor Author

@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...

@benmccann
Copy link
Contributor

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

@danielgindi
Copy link
Contributor Author

Well that may sound weird, but when you're dealing with mixed languages this tends to be an amazing feature.
We could be displaying everything in an RTL manner, but then have the legend items contain mainly LTR content. In such a case we will still layout the legend from right-to-left, but let the text flow from left to right to keep punctuation in the correct positions.

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.
(People tend to flip punctuation positions in movie subtitles to solve this, while I wrote this one: https://jsfiddle.net/danielgindi/ndjwv45x/ to keep the subtitles correct and displaying RTL on all players...)

@danielgindi
Copy link
Contributor Author

@benmccann Also, for changing the text direction for the whole chart - the user simply needs to add direction: rtl to the canvas' CSS. So a global toggle for that already exists :-)

Copy link
Member

@kurkle kurkle left a 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.

src/helpers/helpers.rtl.js Outdated Show resolved Hide resolved
src/helpers/helpers.rtl.js Outdated Show resolved Hide resolved
src/helpers/helpers.rtl.js Outdated Show resolved Hide resolved
src/helpers/helpers.rtl.js Show resolved Hide resolved
src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
@danielgindi
Copy link
Contributor Author

Your CI fails for some reason... 🤔

@benmccann
Copy link
Contributor

@danielgindi I've fixed the CI in #6493. This PR is passing now

@benmccann
Copy link
Contributor

@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
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Implemented RTL support for legends and tooltips
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

6 participants