FIX and Add options to chart2music plotly integration #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ands Add 'decimal' and 'thousands' formats for brazilian portuguese locale file
Adding a closed caption div to plotly interefered with plotly's management of it. We now start by default outside of the generated plotly divs. But we add options to users can a) generate it with any id/class. Or specify that it already exists and use an existing element by id.
…into pikul-music-import-promise
Passing named empty arrays to chart2music makes it angry during validation.
accessibility.js takes configuration variables from `plot_config.js`, modifies them, and passes them directly to `chart2music`. The problem is that assigning an object to a new variable doesn't copy it, so any change to that variable also changes the original object. Plotly uses changes (or lack thereof) in the config variables to decide if it can shortcut certain redraw steps. It cannot if config variables change. So if `accessibility.js` is run on every redraw, the config variables will look different (even if they are not), and plotly will always take the long way around during its rerendering. That being said, this solution here may need more thought. Questions like: when _should_ chart2music ask plotly to do a full redraw? Should it ever? Would it do that by changing a config it has access to? Will it ever change plotly config variables frequently.
I can't think of instance where 1) the caption div wouldn't be specified by the user 2) would maybe do some really fancy interaction w/ the graph 3) be completely invisible because its meant to be read outloud anyway Either way, the generation strategy needs work. * Tests now create their closed caption div
|
I can give you access here, will do that! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Green!
Not really sure how we should proceed, if you want to keep on approving my pull requests so I can add to your pull request or if you give me permission to post to your repo here (is that possible?). It's up to you.
plot_config.jsweren't loading during testing, so we check and reinforce at runtime. Don't love this.I will post a larger comment in the pull request right now about how to move forward this.
PS. saving this link because otherwise pull requests are hard to make....
aliwelchoo:plotly.js:chart2music...geopozo:plotly.js:pikul-music-import-promise