Skip to content
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

[main.js] add missing checks for htmlNode #289

Merged
merged 1 commit into from
Feb 16, 2017
Merged

[main.js] add missing checks for htmlNode #289

merged 1 commit into from
Feb 16, 2017

Conversation

pkra
Copy link
Contributor

@pkra pkra commented Feb 8, 2017

Fixes #288

pkra added a commit that referenced this pull request Feb 8, 2017
pkra added a commit that referenced this pull request Feb 8, 2017
* improve selectors
* update tests
part of #289
@pkra pkra added this to the What comes next -- towards v1.0 milestone Feb 8, 2017
@dpvc
Copy link
Member

dpvc commented Feb 14, 2017

This looks OK, but I think that there may need to be one more change in order to handle requests for css when there is no request for html or htmlNode. You may want to move line 594 to the top of that function, right before line 566.

@pkra
Copy link
Contributor Author

pkra commented Feb 14, 2017

Ah! And I'm guessing

var renderer = ( (data.html || data.htmlNode) ? "CommonHTML" : "SVG");
needs to check for css as well so that typesetting happens with CHTML and the styles get generated, right?

@dpvc
Copy link
Member

dpvc commented Feb 14, 2017

No, the styles are obtained just by loading the output jax, and that always happens. So you don't need to typeset using CHTML to get that. (I haven't tested that, but I'm pretty sure that's true.)

@pkra
Copy link
Contributor Author

pkra commented Feb 15, 2017

I haven't tested that, but I'm pretty sure that's true.

I've tested it and it fails for me, see #292. I think we should merge this and deal with the other issue separately.

@dpvc
Copy link
Member

dpvc commented Feb 15, 2017

Since the fix for data.css is in the same line (687) as this change, I'd say doing as part of this PR would be reasonable.

@pkra
Copy link
Contributor Author

pkra commented Feb 15, 2017

In the interest of concise commits, can I go ahead and merge?

@dpvc
Copy link
Member

dpvc commented Feb 15, 2017

OK, sure.

@pkra pkra merged commit 9b4ef93 into develop Feb 16, 2017
@pkra pkra deleted the issue288 branch February 16, 2017 08:56
pkra added a commit that referenced this pull request Feb 21, 2017
* [main.js] getHTML: add aria-label to correct childnode
Resolves #289

* [main.js] GetHTML: make handling of speakText more robust
* improve selectors
* update tests
part of #289

* address review comments

* fix whitespace
pkra added a commit that referenced this pull request Feb 21, 2017
* [main.js] getHTML: add aria-label to correct childnode
Resolves #289

* [main.js] GetHTML: make handling of speakText more robust
* improve selectors
* update tests
part of #289

* address review comments

* fix whitespace
pkra added a commit that referenced this pull request Mar 13, 2017
* [main.js] getHTML: add aria-label to correct childnode
Resolves #289

* [main.js] GetHTML: make handling of speakText more robust
* improve selectors
* update tests
part of #289

* address review comments

* fix whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants