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

Navis Slideshow expand-on-click styles, again. #1695

Merged
merged 16 commits into from
Apr 25, 2019
Merged

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Apr 25, 2019

Changes

Testing to be completed pre-merge:

  • on MST: works locally; staging experienced a deploy problem with git push and we're talking with the host.
  • on C
  • fullwidth block
  • align left/right/center/none block
  • gallery block?
  • Classic Editor slideshow gallery shortcode
  • Featured Gallery in Largo

@benlk benlk added type: bug status: in progress category: styles affects lots of styles, requiring visual testing category: gutenberg Relating to general Gutenberg compatibility labels Apr 25, 2019
@benlk benlk added this to the 0.6.3 milestone Apr 25, 2019
@benlk benlk requested a review from joshdarby April 25, 2019 03:03
@benlk benlk self-assigned this Apr 25, 2019
@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Bug that appeared in testing: An image in a block that's aligned full will revert to single-column width when the gallery closes.

Before:
Screen Shot 2019-04-25 at 12 20 33

<figure class="wp-block-image alignfull"><img src="http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-1170x780.jpg" alt="Two genders do the bopping" class="wp-image-129823" srcset="http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-1170x780.jpg 1170w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-336x224.jpg 336w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-768x512.jpg 768w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-771x514.jpg 771w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h.jpg 1600w" sizes="(max-width: 1170px) 100vw, 1170px"><figcaption>This is a full-width image. It should be preceded by a single paragraph tag.</figcaption></figure>

Upon click:
Screen Shot 2019-04-25 at 12 20 36

<figure class="wp-block-image alignfull navis-slideshow navis-single navis-full" style="max-width: 100%;"><span class="navis-before">X</span><img src="http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-1170x780.jpg" alt="Two genders do the bopping" class="wp-image-129823" srcset="http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-1170x780.jpg 1170w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-336x224.jpg 336w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-768x512.jpg 768w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-771x514.jpg 771w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h.jpg 1600w"><figcaption>This is a full-width image. It should be preceded by a single paragraph tag.</figcaption></figure>

After closing:
Screen Shot 2019-04-25 at 12 20 41

<figure class="wp-block-image alignfull" style="max-width: 100%;"><img src="http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-1170x780.jpg" alt="Two genders do the bopping" class="wp-image-129823" srcset="http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-1170x780.jpg 1170w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-336x224.jpg 336w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-768x512.jpg 768w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h-771x514.jpg 771w, http://mstoday.test/wp-content/uploads/2018/02/24496652921_1108fff3b7_h.jpg 1600w" sizes="(max-width: 1170px) 100vw, 1170px"><figcaption>This is a full-width image. It should be preceded by a single paragraph tag.</figcaption></figure>

This appears to be because style="max-width: 100%;" is added to the gallery parent in the initial click:

https://github.com/INN/largo/blob/c487ba7b2ec3b455cc519f4f85421c3dcc310bf0/lib/navis-slideshows/js/navis-slideshows.js#L231-L240

But I'm not sure why the gallery.attr call in the close handler isn't removing the style attribute:

https://github.com/INN/largo/blob/c487ba7b2ec3b455cc519f4f85421c3dcc310bf0/lib/navis-slideshows/js/navis-slideshows.js#L145-L155

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

The attrs argument passed in this case are:

 ["(max-width: 1170px) 100vw, 1170px", undefined, undefined, undefined]

which based on the docs in https://api.jquery.com/attr/#attr2, appears to be undefined behavior when undefined is passed as the value for .attr( attributeName, value ).

  • if typeof value === 'undefined': value = null;

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

@joshdarby this is ready for review

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Are we worried that the user can scroll the page when the popup is open, and can thereby trigger the sticky nav to appear, covering the slideshow's close button?

@joshdarby
Copy link
Collaborator

joshdarby commented Apr 25, 2019

Are we worried that the user can scroll the page when the popup is open, and can thereby trigger the sticky nav to appear, covering the slideshow's close button?

@benlk I'd think we'd probably want to disable scrolling when the popup is triggered. That might also help with our other issues of overlapping menus and whatnot.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Also, with a Gallery block, the image is cropped:

Screen Shot 2019-04-25 at 13 36 19

Screen Shot 2019-04-25 at 13 36 14

… Gallery blocks not having correct zoom of images
@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Current to-do list:

  • stop scroll when the modal is open from an image block
  • stop scroll when the modal is open from a gallery block
  • stop scroll when the modal is open from post featured gallery
  • prevent overzoom of images when modal is open from a gallery block

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Gallery Block has margin-left:

Screen Shot 2019-04-25 at 14 02 21

  • CSS fix for this

@joshdarby
Copy link
Collaborator

@benlk One more issue with this that I noticed is that if you have an image expanded and start to resize the window, the sticky nav pops up again. Maybe we should add a check to it to not display if any figure has the .navis-full class.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Or we should change the hide/show functions in this JS to hide a parent of the sticky nav element that's currently being shown/hidden.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Regarding the gallery image cropping, this isn't a JS fix; it's a CSS fix for this monstrosity of a default style:

.wp-block-gallery.is-cropped .blocks-gallery-image a,
.wp-block-gallery.is-cropped .blocks-gallery-image img,
.wp-block-gallery.is-cropped .blocks-gallery-item a,
.wp-block-gallery.is-cropped .blocks-gallery-item img {
    height: 100%;
    flex: 1;
    -o-object-fit: cover;
    object-fit: cover;
}

The 100% height I'm not sure we should change, but looking at object-fit we could use object-fit: scale-down; so that small images aren't blown up and large images don't overflow.

object-fit doesn't work in IE though, which is probably why the 100% height.

@joshdarby
Copy link
Collaborator

@benlk One more: On the sandbox, whenever I click to expand an image inside the Full Width Gallery Block, I just get a white screen with no exit button.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

a white screen with no exit button.

Is that from clicking on one of the light-grey images?

If it is, and if this is a contrast issue, should we do something to make sure that this X is more visible?

Screen Shot 2019-04-25 at 14 38 45

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Nope, this is not just a contrast issue.

  • image in full width gallery block,
  • "at if you have an image expanded and start to resize the window, the sticky nav pops up again. " change the show/hide element on the sticky nav

@joshdarby
Copy link
Collaborator

joshdarby commented Apr 25, 2019

Is that from clicking on one of the light-grey images?

If it is, and if this is a contrast issue, should we do something to make sure that this X is more visible?

@benlk Yes, I did click on one of the light gray images but I don't believe it's a contrast issue. If you click on one of the light gray images above in the Three Column, Four Images block, you're still able to see the exit button.

I also tried replacing one of the images with a darker image and it still showed a white screen with no exit.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Wit that white-page error, there are clearly elements on the page:

Screen Shot 2019-04-25 at 14 53 11

They're just ... not showing up?

@joshdarby
Copy link
Collaborator

joshdarby commented Apr 25, 2019

@benlk I think it's the transform attribute on the .alignfull class on the parent wp-block-gallery messing with it. Removing that attribute will make the image appear.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

The transform problem here is another instance of #1685, which means that we need a general solution for this. Which should be doable.

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Hmm, the change in 4ddfab0 doesn't seem to have taken when I deployed that to staging.

  • cachebusters ahoy!

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

Note: When pushing LESS changes, remember to go to Dashboard > Appearance > CSS Variables and click "Save Variables".

@benlk
Copy link
Collaborator Author

benlk commented Apr 25, 2019

@benlk benlk merged commit c41e28e into 0.5-dev Apr 25, 2019
@benlk benlk deleted the 1664-navis-post-1675 branch April 25, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: gutenberg Relating to general Gutenberg compatibility category: styles affects lots of styles, requiring visual testing status: in progress type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants