-
-
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?
Changes from 13 commits
6fb9c65
87b4c5c
64ae505
d37d24f
d0467bb
3a73776
b4df959
97b608f
6205d62
7e40cde
6d9185b
935cf54
44deae2
c38ab67
c6a27be
cac174c
159ca1b
7467a47
08b820b
41b0fb9
94e25c3
61ea8ce
c9a0b91
cc92639
7baa0d3
3127dee
2429e55
00572e8
30f4e11
247b301
d08adfb
dbce5da
e1f9563
3f4ab54
92d97b7
6b2dcc9
b5bb517
25ca44a
7328324
00b2750
b04bb17
d4bd71b
af2ff80
80b4cf3
ee99f35
3841640
d84d108
0c43407
4c75035
3c49ed6
f7be666
4a260af
f67c513
41baab9
26754b7
ed18907
1e4bcef
2c1abf4
614fb8b
5f5e599
fe9736e
59ad5c6
d18c584
043d408
129eb3b
acc8517
c5ad240
0113d90
771029e
bfbbbf1
cadfd7f
56e582d
8e55f98
86d75d8
15671b0
057e067
44f7ad3
8e3b62b
aed1e02
1777fba
3a0384f
dba0b53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
"use strict"; | ||
import c2mChart from "chart2music"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. For es modules, we have a config rule in place. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
export function enable (gd) { | ||
|
||
const c2mData = {}; | ||
const labels = []; | ||
|
||
const fullData = gd._fullData; | ||
|
||
for(var i = 0; i < fullData.length; i++) { | ||
var trace = fullData[i] ?? {}; | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. In scatter traces with |
||
for(var p = 0; p < y.length; p++) { | ||
traceData.push( | ||
{ | ||
x: x.length > 0 ? x[p] : p, | ||
y: y[p], | ||
label: text[p] ?? p | ||
}) | ||
} | ||
c2mData[name] = traceData; | ||
labels.push(name); | ||
} | ||
} | ||
else { | ||
// 'Accessibility not implemented for trace type: ' + trace.type | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Chart2Music v1.12 now has an |
||
} | ||
} | ||
|
||
var closed_captions = document.createElement('div'); | ||
closed_captions.id = 'cc'; | ||
closed_captions.className = 'closed_captions'; | ||
gd.appendChild(closed_captions); | ||
|
||
const { | ||
title: {text: title_text = ''} = {}, | ||
xaxis: {title: {text: xaxis_text = ''} = {}} = {}, | ||
yaxis: {title: {text: yaxis_text = ''} = {}} = {}, | ||
} = gd._fullLayout; | ||
|
||
c2mChart({ | ||
title: title_text, | ||
type: "line", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this PR is for scatter plots, you probably want to have 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for supporting our project, @julianna-langston |
||
axes: { | ||
x: { | ||
label: xaxis_text | ||
}, | ||
y: { | ||
label: yaxis_text | ||
} | ||
}, | ||
element: gd, | ||
cc: closed_captions, | ||
data: c2mData, | ||
options: { | ||
onFocusCallback: ({slice, index}) => { | ||
Plotly.Fx.hover(gd, [{ | ||
curveNumber: labels.indexOf(slice), | ||
pointNumber: index | ||
}]) | ||
}, | ||
...gd._context.chart2musicOptions | ||
}, | ||
info: gd._context.chart2musicInfo | ||
aliwelchoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
}; |
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
plotly.js/webpack.config.js
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