thegetty / cva10

"Corpus Vasorum Antiquorum, Fascicule 10", by Despoina Tsiafakis
1 stars 0 forks source link

leaflet fixes #46

Closed naeluh closed 5 years ago

naeluh commented 5 years ago

@geealbers fixes https://github.com/thegetty/cva10/issues/40

For https://github.com/gettypubs/quire/issues/171 I think that may be layering issue the elements are there they are just behind the full screen I think If I change it so the image count and pager in the figure element this would come with the fullscreen mechanism this is untested but I believe it would work.

Let me know if that makes sense or if you want to go another route.

thanks!

geealbers commented 5 years ago

Thanks @naeluh. I think I see what you're saying, but I also think the elements like the figure count and caption are actually outside of the div that the fullscreen is applied to. And so not just hidden behind. In the screen shot below, I think it's the highlighted div that's fullscreen, but the elements we want to show are in divs below and buttons above it. Does that make sense, and am I right that the fullscreen would be applied just to that one div and not to any other element?

Screen Shot 2019-06-25 at 3 54 32 PM

I'm also having trouble what you actually changed in the commit because the diff is showing so much that's just reformatting. So I might totally off base. But I did try it locally and I'm still not seeing the controls.

naeluh commented 5 years ago

Hi @geealbers what controls ? Sorry about that I have prettier turned on. Yes I think the caption and the number count should be inside the figure. I have not added that yet. I will try and see if it works. Does that make sense?

geealbers commented 5 years ago

So ideally, everything that's in this screenshot, should appear whether looking at the image in a normal browser window, or full screen mode.

Screen Shot 2019-06-25 at 4 19 15 PM
naeluh commented 5 years ago

Hi @geealbers ok let me see what I can do! thanks! looks like pull request preview is work now https://5d12b87b57f61d0009ae2e79--cva10.netlify.com/

geealbers commented 5 years ago

Yes! This is exactly right.

A small change. Can you make it cursor: pointer when clicking or hovering over the left and right arrows rotating through the images?

And for me, on Safari on Mac and Chrome on PC, it's working as expected, but not expanding the full window width.

Screen Shot 2019-06-26 at 4 05 36 PM

Also on IE 11, it doesn't seem to be working at all. Images aren't loading and the controls are missing or unresponsive. I can see a number of other unrelated issues too though (like with the sidebar menu) so I think I need to investigate more and probably open a whole new issue for them.

naeluh commented 5 years ago

Hi Greg - ok interesting I am not getting that width issue at all can you run quire preview a couple of times and see if it clears up that width issue there might be some cache styling. I will do some testing and see what is up with ie.

naeluh commented 5 years ago

ok I am seeing the width issue in safari -- I will add a patch for this

naeluh commented 5 years ago

Hi Greg ok the last commit should have all the fixes for ie including the menu. https://github.com/thegetty/cva10/pull/46/commits/bca05121e562bd8a75e645c20705ce1c7bb8ca9a Let me know if you notice anything else. Thanks!