-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
add Chart2Music keyboard and sound accessibility features #6680
base: master
Are you sure you want to change the base?
Conversation
|
Excited for this! @archmoj thoughts about packaging? Is a 50kb increase in minified bundle acceptable or should we keep |
package.json
Outdated
@@ -77,6 +77,7 @@ | |||
"@turf/bbox": "^6.4.0", | |||
"@turf/centroid": "^6.0.2", | |||
"canvas-fit": "^1.5.0", | |||
"chart2music": "^1.11.0", |
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
include: /node_modules[\\\/](buffer|d3-color|d3-interpolate|is-mobile)[\\\/]/, |
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
@@ -0,0 +1,71 @@ | |||
"use strict"; | |||
import c2mChart from "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.
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
var {type, x = [], y = [], name = i, text = []} = trace; | ||
if(type === 'scatter') { | ||
var traceData = []; | ||
if ('y' in trace) { |
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
} | ||
else { | ||
// 'Accessibility not implemented for trace type: ' + trace.type | ||
return; |
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
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.
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
|
||
c2mChart({ | ||
title: title_text, | ||
type: "line", |
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
var ccElement = initClosedCaptionDiv(gd, c2mContext.ccOptions); | ||
|
||
// TODO: | ||
// I believe 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.
@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.
); | ||
|
||
if(gd._context.sonification.enabled) seq.push(sonification.enable_sonification); |
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?
} else { | ||
return document.getElementById(config.elId); | ||
} |
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.
} else { | |
return document.getElementById(config.elId); | |
} | |
} | |
return document.getElementById(config.elId); |
var x = trace.x && trace.x.length !== 0 ? trace.x : []; | ||
var y = trace.y && trace.y.length !== 0 ? trace.y : []; | ||
|
||
for(var p = 0; p < Math.max(x.length, y.length); p++) { |
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.
// This codec serves for one x axis and one y axis | ||
|
||
function test(trace) { | ||
if (!trace || !trace.type || trace.type !== 'scatter') return null; |
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 |
Use data and layout to call the chart2music API inside makePlot.
This pull request is for scatter plots only