-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add Chart2Music keyboard and sound accessibility features #6680
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
Conversation
- Closed captions element is required to be visible on the screen to function so need to find a way to integrate it more cleanly
- Need to rebase
Excited for this!
FYI @julianna-langston @spmealin - I think we can sort out most of what we need but your feedback is of course welcome!
@archmoj thoughts about packaging? Is a 50kb increase in minified bundle acceptable or should we keep chart2music
external like MathJax
? If we did make it external we'd probably want it included by default in plotly.py output, and we could have Dash detect and autoload it, but plain plotly.js users would need to explicitly load it
package.json
Outdated
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.
Nice MIT library.
To transpile it to es5, please add it to
Line 25 in 149c895
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.
Thank you! Knew there'd be something simple I was missing so I could get this into the same style
src/plot_api/accessibility.js
Outdated
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.
To pass current es5 build tests, this file should be written in es5 style similar other files to src
.
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.
Do we still need to be writing in es5? Won't webpack take that and give us the desired output? I'd love it if we can start accepting (and eventually converting existing code to) more modern syntax.
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 es modules, we have a config rule in place.
I did try to add a rule to transpile this file during build processes; but npm run build
wasn't successful in the past hour.
If one could help with setting up the webpack config, it would be great 🙏
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.
Thank you for these tips again! I've made some attempts at this (putting chart2music and/or accessibility.js into the webpack config but struggling with "Cannot find module 'chart2music'" whenever I use require instead of import. I think I'm still missing something
src/plot_api/accessibility.js
Outdated
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.
In scatter traces with orientation: 'h'
, we should use trace.x
instead of trace.y
.
src/plot_api/accessibility.js
Outdated
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.
Perhaps we should use continue
instead of return
here.
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.
Yes I think so. There's some overview text c2m reads out when you first focus on the chart, right? Perhaps we can have any skipped traces briefly mentioned there?
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 can look into this. Need to look into where that closed captions text element goes in general, not sure about outputting our own messages into it, it's more than just the overview at the start, also reads off the data as you navigate it.
But this general point is a good one, ideally want to be able to support more than just scatter traces and want to support figures containing unsupported traces alongside supported ones.
Then there's also the opportunity for a config option to say which traces should be in accessibility mode
@julianna-langston
julianna-langston
Jul 25, 2023
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.
This is an interesting use case. I've created a feature request for chart2music to add config so that you can specify skipped traces (aka unsupported layers). If you think that would work for you, I'll go ahead and get started on a PR for chart2music.
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.
@julianna-langston that would be great! I think that's going to be really important for this project, so that plotly.js and Dash users who care about this can turn on chart2music in all of their charts, knowing that the result will not always be complete but will always be as useful as it can be and will tell them what they're missing. And then over time as both the plotly.js and the chart2music sides add more features, existing applications will automatically improve.
@julianna-langston
julianna-langston
Aug 28, 2023
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.
Chart2Music v1.12 now has an unsupported
chart type. Examples and documentation
src/plot_api/accessibility.js
Outdated
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.
If this PR is for scatter plots, you probably want to have type: "scatter"
.
In the future, if someone wants to support more chart types, chart2music supports an array. So, if you have 2 traces, 1 scatter and 1 line, you could convert that to chart2music's type: ["line", "scatter"]
. Just fyi.
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.
Thank you. I think I'll end up extending the scope to as many types as we can
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.
Thank you for supporting our project, @julianna-langston
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.
@archmoj Is that accurate that title or title.text could contain the information needed?
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.
The preferred way of setting a title is via title.text
not title
.
Setting title
to a string is discouraged(& deprecated) but may still widely used in our codes.
We may handle case of title
in case title is set to a string instead of an object.
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.
We should drop support for that!
This reverts commit 129eb3b.
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.
IS sonification
key always present?
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.
We could simply return and drop else.
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.
Let's put Math.max(x.length, y.length)
in a variable before the loop and use it inside the loop.
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.
Does it for scattergl
?
Last commit fixes #7193
Uh oh!
There was an error while loading. Please reload this page.
Use data and layout to call the chart2music API inside makePlot.
This pull request is for scatter plots only
Demo on codepen: https://codepen.io/marthacryan/pen/wvVMrEe