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

[MRG + 2] [MAINT] Update to Sphinx-Gallery 0.1.7 #7986

Merged
merged 5 commits into from Jan 17, 2017

Conversation

@Titan-C
Copy link
Contributor

@Titan-C Titan-C commented Dec 6, 2016

latest release of Sphinx-Gallery

@jnothman
Copy link
Member

@jnothman jnothman commented Dec 6, 2016

Could you please point to a change-log? Thanks

@amueller
Copy link
Member

@amueller amueller commented Dec 6, 2016

merge if tests pass, I guess? Not sure what should be reviewed? Can you add an entry to whatsnew?

@lesteve
Copy link
Member

@lesteve lesteve commented Dec 6, 2016

Not sure what should be reviewed?

I guess having a glance at the gallery and make sure nothing looks off is a good start: https://7552-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/auto_examples/index.html

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Dec 6, 2016

The relevant changes of Sphinx-gallery for Scikit-learn from version 0.1.4 to 0.1.6 are
v0.1.6

Bug Fixes

  • Sphinx-Gallery now raises an exception if the matplotlib bakend can not be set to 'agg'.

  • Fix backreferences.identify_names when module is used without attribute

  • Raise FileNotFoundError when README.txt is not present in the main directory of the examples gallery(#164). Also include extra empty lines after reading README.txt to obtain the correct rendering of the html file.(#165)

  • CSS. Now a tooltip is displayed on the source code blocks to make the doc-resolv functionality more discorverable. Function calls in the source code blocks are hyperlinks to their online documentation.
    Download buttons have a nicer look across all themes offered by Sphinx

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Dec 6, 2016

Ok this one looks a bit ugly a CSS conflict. The tooltip for links to functions documentation appears below the sidebar to hide the left menu.
code_tooltip

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Dec 6, 2016

I pushed a fix for the tooltip alignment. Now it left aligns to with the world so that is not anymore overlapping with the sidebar button. For reasons that go beyond my understanding on CSS raising the z-index of the tooltip did not help to put it on top of the sidebar. The highest index is from the bootstrap themes and is 1051, but even putting the tooltip to 2000 did not change anything.

@@ -209,7 +209,6 @@ a.sphx-glr-code-links[tooltip]:hover:before{
display: inline-block;
text-align: center;
text-indent: 0;
margin-left: -5em;
margin-left: 0; /* Has to be zero to avoid overlapping with sidebar */

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 6, 2016
Member

Is this change a backport of something in sphinx_gallery, or is it a modification specific for scikit-learn?

I am asking because I am worried that if it is specific to scikit-learn, next time that we upgrade, we will break something.

This comment has been minimized.

@Titan-C

Titan-C Dec 6, 2016
Author Contributor

It is a modification done in place for now for scikit learn. Which I believe to be reasonable enough to transfer to sphinx-gallery.
Code tooltips are a recent feature in Sphinx-Gallery from your last PR as you wanted to make their use more explicit for Scipy-lectures code examples.
From a personal point of view, I see little advantage on trying to make them look a bit centered relative to the link and left aligning seems good enough. I was once slightly annoyed they apear bellow the link, because if 2 consecutive lines have function calls, the first tooltip will block the second link and you have to move the mouse around to make them disappear and select the second function link. Then I thought making them inline would be better, and then again I thought that a nested function call I would experience the same issue, so I let it be.

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 6, 2016
Member

OK. I'll have a look once CircleCI is finished running, and we should either consider porting this back to sphinx gallery, or making the CSS change in a scikit-learn specific file.

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 6, 2016
Member

OK, looks good. +1 for backport into sphinx-gallery

@amueller
Copy link
Member

@amueller amueller commented Dec 6, 2016

LGTM

@amueller amueller changed the title [MAINT] Update to Sphinx-Gallery 0.1.6 [MRG + 1] [MAINT] Update to Sphinx-Gallery 0.1.6 Dec 6, 2016
@jnothman
Copy link
Member

@jnothman jnothman commented Dec 7, 2016

@Titan-C: for future reference, we have shortcuts to Circle CI artifacts like http://scikit-learn.org/circle?7560/auto_examples

I'm not entirely happy with those tooltips. I'm okay to merge this now, despite, but I'd rather:

  • "Link to" -> "View"
  • Using <a title="..."> rather than some custom intrusive tooltip.
@raghavrv raghavrv added the Build / CI label Dec 9, 2016
@raghavrv
Copy link
Member

@raghavrv raghavrv commented Dec 9, 2016

I guess having a glance at the gallery and make sure nothing looks off is a good start: https://7552-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/auto_examples/index.html

I checked for around 20 examples, it looks fine to me...

@raghavrv raghavrv changed the title [MRG + 1] [MAINT] Update to Sphinx-Gallery 0.1.6 [MRG + 2] [MAINT] Update to Sphinx-Gallery 0.1.6 Dec 9, 2016
@amueller
Copy link
Member

@amueller amueller commented Dec 9, 2016

Do we want to wait with this until the sphinx 1.5 bug is fixed? sphinx-gallery/sphinx-gallery#137

@lesteve
Copy link
Member

@lesteve lesteve commented Dec 9, 2016

Do we want to wait with this until the sphinx 1.5 bug is fixed? sphinx-gallery/sphinx-gallery#137

I would say yes.

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Dec 11, 2016

I'm keeping track on what shall get in the next Sphinx-Gallery bug fix release apart from bug fix to Sphinx v1.5 new changes.

Concerning code tooltips @jnothman

I'm not entirely happy with those tooltips. I'm okay to merge this now, despite, but I'd rather:

  • "Link to" -> "View"
  • Using rather than some custom intrusive tooltip.
@jnothman
Copy link
Member

@jnothman jnothman commented Dec 12, 2016

Thanks @Titan-C, I don't really understand why title is breaking with readthedocs, or why that should not be considered a bug on the part of readthedocs (title is, of course, the correct way to do this in Semantic HTML)

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Dec 12, 2016

Title does not only break in readthedocs. It breaks on all themes. I retested it last night, the reason is the browser itself will popup the info box with the style defined by the browser. When @GaelVaroquaux tried to put CSS style to this box, the styled box appears first and then a little later the browser pops again its own box. For that reason there is duplication.
There is of course the option of not to style this at all and let the browser handle the title attribute.
On the same thread of issue sphinx-gallery/sphinx-gallery#145 (comment), Gaël references that title has no effect on web readers for blind people so there is no true advantage for the title attribute.

@jnothman
Copy link
Member

@jnothman jnothman commented Dec 12, 2016

Titan-C added 2 commits Dec 6, 2016
Tooltip starts with same left alignment as link. This is to avoid
tooltip appearing below menu hide sidebar button. The z-index is not
working as should putting it on top of the sidebar.
@Titan-C Titan-C force-pushed the Titan-C:glr_maint branch from f50a497 to 473459c Jan 16, 2017
@lesteve
Copy link
Member

@lesteve lesteve commented Jan 16, 2017

This does not appear to be an umodified 0.1.7 sphinx-gallery version, can you fix this? Running copy_sphinxgallery.sh and diffing I get:

diff --git a/doc/sphinxext/sphinx_gallery/_static/gallery.css b/doc/sphinxext/sphinx_gallery/_static/gallery.css
index a7e2890..9de9b97 100644
--- a/doc/sphinxext/sphinx_gallery/_static/gallery.css
+++ b/doc/sphinxext/sphinx_gallery/_static/gallery.css
@@ -195,11 +195,11 @@ a.sphx-glr-code-links:hover{
     text-decoration: none;
 }
 
-a.sphx-glr-code-links[tooltip]:hover:before{
+a.sphx-glr-code-links[title]:hover:before{
     background: rgba(0,0,0,.8);
     border-radius: 5px;
     color: white;
-    content: attr(tooltip);
+    content: attr(title);
     padding: 5px 15px;
     position: absolute;
     z-index: 98;
diff --git a/doc/sphinxext/sphinx_gallery/docs_resolv.py b/doc/sphinxext/sphinx_gallery/docs_resolv.py
index b2757d2..12346b5 100644
--- a/doc/sphinxext/sphinx_gallery/docs_resolv.py
+++ b/doc/sphinxext/sphinx_gallery/docs_resolv.py
@@ -358,7 +358,7 @@ def _embed_code_links(app, gallery_conf, gallery_dir):
 
     # patterns for replacement
     link_pattern = ('<a href="%s" class="sphx-glr-code-links" '
-       'tooltip="Link to documentation for %s">%s</a>')
+                    'title="Link to documentation for %s">%s</a>')
     orig_pattern = '<span class="n">%s</span>'
     period = '<span class="o">.</span>'
@lesteve lesteve changed the title [MRG + 2] [MAINT] Update to Sphinx-Gallery 0.1.6 [MRG + 2] [MAINT] Update to Sphinx-Gallery 0.1.7 Jan 16, 2017
@lesteve
Copy link
Member

@lesteve lesteve commented Jan 16, 2017

Also can you have [doc build] somewhere in your commit message in order to force generation of the full html documentation?

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Jan 16, 2017

This does not appear to be an umodified 0.1.7 sphinx-gallery version, can you fix this? Running copy_sphinxgallery.sh and diffing I get

Indeed doing this update I noticed this. Version 0.1.7 has still tooltip and it never goes to title. But I was testing with style at that time too. For some reason I pushed to PyPI a version with title, that is wrong. I'm currently looking how to change the file in PyPI

@lesteve
Copy link
Member

@lesteve lesteve commented Jan 16, 2017

Yes I'm required to have a new version. I'll test first in test PyPI to see how the ordering goes

Unless the code you uploaded for 0.1.7 has a big regression, I would not bother too much about it since 0.1.8 is going to be released in not such a long time IIRC.

@Titan-C Titan-C force-pushed the Titan-C:glr_maint branch from 51cc9fa to d61464f Jan 16, 2017
@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Jan 16, 2017

Unless the code you uploaded for 0.1.7 has a big regression, I would not bother too much about it since 0.1.8 is going to be released in not such a long time IIRC.

the PyPI version of 0.1.7 uses the title CSS atribute. No one has complained, but that gives this problem sphinx-gallery/sphinx-gallery#145 (comment).

Yes 0.1.8 shall be out as soon as the relative path PR sphinx-gallery/sphinx-gallery#190 is approved. Sphinx-Gallery 0.1.8 would contain the style changes requested here, of dropping the tooltip in favour of title and letting the browser handle the style of it.

@lesteve
Copy link
Member

@lesteve lesteve commented Jan 16, 2017

HTML doc for the CircleCI build can be found here. I looked at the gallery and I can't find any obvious issues.

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 17, 2017

Are the download buttons meant to be side-by-side like on sphinx-gallery.readthedocs.io? They're appearing one above the other.

@lesteve
Copy link
Member

@lesteve lesteve commented Jan 17, 2017

Are the download buttons meant to be side-by-side like on sphinx-gallery.readthedocs.io? They're appearing one above the other.

Hmmm not sure to be perfectly honest.

I would still be in favour of merging this PR to be able to use sphinx 1.5 and have CircleCI running fine. We can sort out cosmetic in a later update of sphinx-gallery.

@GaelVaroquaux
Copy link
Member

@GaelVaroquaux GaelVaroquaux commented Jan 17, 2017

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 17, 2017

@Titan-C
Copy link
Contributor Author

@Titan-C Titan-C commented Jan 17, 2017

@ogrisel
Copy link
Member

@ogrisel ogrisel commented Jan 17, 2017

+1 for merging this now to fix CI on master ASAP and tackle the cosmetic link layout issue in a latter PR.

@lesteve
Copy link
Member

@lesteve lesteve commented Jan 17, 2017

Merging this one, thanks a lot @Titan-C !

@lesteve lesteve merged commit bddda7b into scikit-learn:master Jan 17, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 17, 2017

raghavrv added a commit to raghavrv/scikit-learn that referenced this pull request Jan 18, 2017
@Titan-C Titan-C deleted the Titan-C:glr_maint branch Jan 22, 2017
sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Jun 20, 2017
NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.