-
Notifications
You must be signed in to change notification settings - Fork 842
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
docs(elastic charts docs): elastic-chart a11y features into eui docs #4916
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
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.
Thanks for getting these started!
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
@rshen91 Do you want to add goal chart docs to this PR or a follow up? |
Oh good call - I can definitely add the goal improvement changes. 🙌🏻 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Thanks for merging rshen91#3. The idea of the PR is to have the hidden content examples outside of the demo table: Another thing that I noticed is in dark mode some demo elements don't have enough contrast. I think once we fix this from a design perspective we're good to merge. |
return ( | ||
<Fragment> | ||
<EuiTitle size="xs"> | ||
<h3 id={id}>Example chart with texture fills</h3> |
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.
For consistency with the other demos, we should center this title.
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.
Good catch thank you! 40fed7f
Unfortunately, the bullet graph doesn't yet have a linkLabels way to change the text color (like with partitions where you can set the linkLabels). It's on our roadmap for when the bullet graph moves out of alpha. Thanks!
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Thanks for 40fed7f. 😄 But I'm still seeing this chart with dark text. The text should be lighter: And for this one, the outer labels and lines should be lighter too: |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
@rshen91 I was trying to figure out the color issue we're having with the pie charts and the link labels. It seems that the issue was originated here #3669 when the To provide an example. If I pass the following colors: partition: {
linkLabel: {
textColor: 'yellow',
},
sectorLineStroke: 'red',
} This is what I get: If I pass partition: {
linkLabel: {
textColor: 'yellow',
},
sectorLineStroke: 'white',
} The pointing lines get Ideally, the pointing lines should match the I also noticed another issue. If we pass the partition: {
linkLabel: {
textColor: colors.euiColorFullShade.rgba,
}
} In light mode it works. But in dark mode this is what I get (the expected I'll open an issue in the Bullet graph
So I added a background that doesn't adapt the color to light/dark theme. And removed the code for adapting the theme. It was not doing anything because the chart doesn't accept it. So I think we're good to merge the PR. 🎉 |
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.
Thanks @rshen91 for all the work and changes! LGTM! 🎉
Tested in Chrome, Safari, Edge and Firefox. I also looked at the code. We had some design limitations described here: #4916 (comment). But for now, this is the best we can do. 😄
Preview documentation changes for this PR: https://eui.elastic.co/pr_4916/ |
Summary
Closes #4875
Updating the accessibility improvements in the elastic-charts section in the EUI docs.
Checklist