Skip to content

Gallery output images have own css class - #210

Merged
choldgraf merged 5 commits into
sphinx-gallery:masterfrom
Titan-C:css_img
Feb 6, 2018
Merged

choldgraf merged 5 commits into
sphinx-gallery:masterfrom
Titan-C:css_img

Conversation

@Titan-C

@Titan-C Titan-C commented Mar 11, 2017

Copy link
Copy Markdown
Member

In response to #209

@Titan-C

Titan-C commented Mar 11, 2017 •

Copy link
Copy Markdown
Member Author

A little survey on preferences: Given that the gallery output images now get an own CSS class. Should we keep Sphinx standard styling directives :align: and :scale: or just move all that to CSS?

@lesteve

lesteve commented Mar 13, 2017

Copy link
Copy Markdown
Member

sphx-glr-li-img does not seem like a great name. Naming suggestion: sphx-glr-single-img and sphx-glr-img. Better names more than welcome.

I am fine moving things to the CSS.

@Titan-C

Titan-C commented Mar 27, 2017

Copy link
Copy Markdown
Member Author

I moved image styles to CSS. Changed to new names sphx-glr-single-img and sphx-glr-multi-img. In the case of multilpe images, they won't be always rescaled to 42% of their size, but have a max-width of 42%. So depending of the availible space they will look bigger or smaller. This makes the output responsive, which looks good. The only feature I start to miss is that when using sphinx's :scale: the image becomes clickable to see the original full size.

@Titan-C
Titan-C requested a review from lesteve April 16, 2017 19:52
@lesteve

lesteve commented Apr 19, 2017

Copy link
Copy Markdown
Member

The only feature I start to miss is that when using sphinx's :scale: the image becomes clickable to see the original full size.

I think you could add a target: in the image directive for both single and multi images, this way they both would be clickable.

Other than this, LGTM.

@Titan-C Titan-C mentioned this pull request Aug 1, 2017
@choldgraf

Copy link
Copy Markdown
Contributor

if I it's not too complex, I agree with @lesteve about adding a target, though I don't think it should be blocking on this PR if it is complicated

@larsoner

larsoner commented Feb 5, 2018

Copy link
Copy Markdown
Contributor

@Titan-C I just hit a use case where this is necessary, as I need to add max-width: 100% to the gallery image CSS to avoid the image spilling over. It seems like this needs a doc update, and possibly the :target: update (though I'm not sure how easy or difficult this would be). Any time for this? Or could I take over?

@Titan-C

Titan-C commented Feb 5, 2018

Copy link
Copy Markdown
Member Author

@larsoner feel free to take over. Although I think we should merge this directly. :target: update is very hard to get working because it takes an url, whereas image takes the path of the file and then moves the image to the _images directory. The correct way to handle that would be extending the image directive in Sphinx, which I'm against because is too complicated to deal with and give maintenance. My alternative would be to solve in the same fashion as I did for the gallery thumbnails only with CSS, maybe by hovering over the image.

@Titan-C

Titan-C commented Feb 5, 2018

Copy link
Copy Markdown
Member Author

I'm proposing a CSS zoom on the images.

@codecov-io

codecov-io commented Feb 5, 2018 •

Copy link
Copy Markdown

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   91.76%   91.76%           
=======================================
  Files          27       27           
  Lines        1833     1833           
=======================================
  Hits         1682     1682           
  Misses        151      151
Impacted Files Coverage Δ
sphinx_gallery/gen_rst.py 94.2% <ø> (ø) ⬆️
sphinx_gallery/tests/test_gen_rst.py 98.48% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 955c41b...454c80e. Read the comment docs.

@larsoner

larsoner commented Feb 5, 2018

Copy link
Copy Markdown
Contributor

I just tried it and for galleries with large images it's difficult to avoid mousing over the image and hitting the zoom behavior, which in turn covers up the narrative. This makes interaction a bit clunky, so -1 on that for me. (Aethestically I also find the changes too distracting / invasive to make it a default.) But otherwise the CSS change of max-width: 100% works well for me.

@larsoner

larsoner commented Feb 5, 2018

Copy link
Copy Markdown
Contributor

@lesteve are you okay with this one as is? Seems like a pretty straightforward CSS flexibility / extension now.

+1 for merge from me, fixes PySurfer doc build nipy/PySurfer#225 (though the new floating buttons also break it :) )

@choldgraf

choldgraf commented Feb 5, 2018 •

Copy link
Copy Markdown
Contributor

+1 on merge - always better to use classes than to hard-code css styles IMO. If nobody protests I'll merge this tomorrow AM unless somebody beats me to it

@choldgraf
choldgraf merged commit 06adfe1 into sphinx-gallery:master Feb 6, 2018
@choldgraf

Copy link
Copy Markdown
Contributor

🎉

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.

5 participants