Added `box-sizing: border-box;` to... #1711

joshdarby · 2019-05-20T13:47:18Z

figcaption and .wp-caption-text elements so that the caption text doesn't overflow from the parent container. Issue #1702

…captions

joshdarby · 2019-05-20T13:51:29Z

@benlk Can you review this and let me know if you think we need to add the box-sizing attribute to any other versions of figcaption and .wp-caption-text?

…captions

benlk · 2019-06-17T23:09:40Z

.wp-block-gallery, which I'll add some styles for in here.

less/inc/images.less

…-gallery; other changes: - make the figcaption no more than 50% of height of container - left align text to match other captions - reduce padding on caption

benlk · 2019-06-17T23:59:09Z

The padding/max-height changes in df15461 are to make sure that with Very Long captions, the caption doesn't fully cover the image.

But the Gutenberg gallery styles here don't match anything in Largo: we generally don't overlay captions over images. Should we try to put the captions beneath the image, like happens with the classic editor's gallery shortcode?

If the gallery shortcode and the classic editor's gallery and the classic editor's image show captions in the expanded slideshow view, should the classic block?

Classic image:

Gallery block:

joshdarby · 2019-06-18T14:41:07Z

But the Gutenberg gallery styles here don't match anything in Largo: we generally don't overlay captions over images. Should we try to put the captions beneath the image, like happens with the classic editor's gallery shortcode?

@benlk I'm not opposed to either way. I'd think if having them below matches Largo styles more, we should go that route.

If the gallery shortcode and the classic editor's gallery and the classic editor's image show captions in the expanded slideshow view, should the classic block?

If 3/4 of them are showing the caption in the expanded view, I'd think the 4th should show it also. Unless you think there's a use case where someone using the theme would want one instance to use that doesn't show the captions in that view?

benlk · 2019-06-18T21:45:56Z

If the gallery shortcode and the classic editor's gallery and the classic editor's image show captions in the expanded slideshow view, should the classic block?

If 3/4 of them are showing the caption in the expanded view, I'd think the 4th should show it also. Unless you think there's a use case where someone using the theme would want one instance to use that doesn't show the captions in that view?

I don't believe there is such a case; let's make those cases consistent.

But the Gutenberg gallery styles here don't match anything in Largo: we generally don't overlay captions over images. Should we try to put the captions beneath the image, like happens with the classic editor's gallery shortcode?

@benlk I'm not opposed to either way. I'd think if having them below matches Largo styles more, we should go that route.

My main worry here is that, since we have Gutenberg's default stylesheet enqueued, Gutenberg will change something upstream will horribly conflict with the styles we apply here, and as a result our sites will complain.

I'd love for there to be a way to get Gutenberg to enqueue its styles per-block, so that we would be able to dequeue the Gallery Block styles and replace those, but then let Gutenberg enqueue the styles for New Shiny Block until we can write custom styles for it.

Should we go ahead and make drastic changes to CSS without dequeueing Gutenberg's styles?

benlk · 2019-06-19T23:11:20Z

Should we go ahead and make drastic changes to CSS without dequeueing Gutenberg's styles?

No.

Therefore, let's not change the gallery block caption display location, yet.

benlk · 2019-06-19T23:11:44Z

Next steps:

Make sure all captions display consistently in slideshow view.

joshdarby · 2019-06-24T15:49:09Z

Make sure all captions display consistently in slideshow view.

@benlk Should they all display the way they do in the classic image or in the Gutenberg block?

benlk · 2019-06-24T15:50:54Z

My personal preference is the classic image view, because that's easier to read than the gradient background.

…captions

also updated single image block expanded view caption styling to match the classic image.

joshdarby · 2019-06-24T17:16:05Z

@benlk Now the captions throughout classic image, image gallery block, and classic gallery shortcode are consistent. The only inconsistency is the known issue of Gutenberg image blocks/galleries not displaying the media credits from issue #1683.

benlk · 2019-06-24T17:38:15Z

Sounds good; can you shove screenshots in this ticket and update the changelog before we merge?

joshdarby · 2019-06-24T17:46:11Z

@benlk

Classic image block:

Classic image gallery:

Gutenberg gallery block:

pullrequest

Looks good to me overall.

Reviewed with ❤️ by PullRequest

less/inc/images.less

benlk · 2019-07-01T02:38:42Z

The background shouldn't be flat transparent; it should be rgba(0,0,0,.7); to match the Navis slideshow styles:

https://github.com/INN/largo/blob/c41e28e6381397be1918184b906ec84bc019745b/lib/navis-slideshows/css/slides.css#L308

Also, the figcaption in wp-block-image in the Navis slideshow state doesn't match these styles; the caption is hidden.

Edit: this is fixed now.

benlk · 2019-07-01T02:51:19Z

When testing images, we need to test all of these separate sets of conditions:

caption state
1. no caption
2. caption
3. media credit
  - name
  - name and org
  - name and org and link
4. caption and media credit
image type
1. classic editor image
2. classic editor gallery shortcode
3. block editor image
4. block editor gallery
alignment options (block and classic)
1. unset
2. left
3. center
4. right
5. wide
6. full

That's ... 6 * 4 * 6 = 144 different potential image types to test. 288 if we include the clicked-on Navis Slideshow as another variable.

We need a plan for testing this stuff systematically, or at least a stock set of markup to test against that generates all those options.

benlk · 2019-07-01T03:11:56Z

Also, working on this changeset and similar ones has made me doubt the usefulness of splitting the Navis Slideshow CSS out from other image-related CSS in Largo; there's a lot of common variables. I'd be in favor of generating it using the Grunt workflow from LESS, where we could set shared variables between the other image code and the slideshow code. The slideshow CSS/JS should still only be enqueued when needed.

benlk · 2019-07-02T18:02:22Z

Another image case to consider:

WordPress gallery widget
Largo image widget
WordPress image widget

pullrequest · 2019-07-05T11:30:42Z

less/inc/blocks.less

+    figcaption {
+      box-sizing: border-box;
+      text-align: left; // override Gutenberg's figcaption centering
+      padding: 10px; // reduce blank space, because clicking on the figcaption doesn't trigger a click on the image to open the gallery.


fix padding (one space is missing).

pullrequest · 2019-07-05T11:30:42Z

lib/navis-slideshows/css/slides.css

@@ -222,11 +223,6 @@ body.single-format-standard .navis-slideshow a.slick-next {
 .navis-slideshow.navis-full.alignnone {
  display: block;
 }
-.wp-block-image.navis-slideshow.navis-full figcaption,
-.wp-block-image .navis-slideshow.navis-full figcaption, /* yes, with and without space */
-.navis-slideshow.navis-full .permalink {


.permalink will be rendered in slideshow after this change?

joshdarby · 2019-07-09T18:04:20Z

@benlk Do you want to get together and figure out a plan for acceptable criteria for getting this PR merged?

benlk · 2019-07-09T22:27:05Z

Yeah, let's set aside time tomorrow to test this.

benlk · 2019-07-15T13:52:32Z

IIRC, the plan we had discussed on that day was:

test against the test content we have available to us now
postpone the Grand Generation of Test Content until a later release

joshdarby · 2019-07-17T18:56:34Z

@benlk I created a sample post here with a

classic block image
classic block gallery
image block
gallery block

What else do you think we should test for this?

…captions

benlk · 2019-07-17T20:15:53Z

Do we know what's going on in Firefox where opening the modal scrolls the browser to the top of the page, triggering the appearance of the sticky nav? #1745

benlk · 2019-07-17T20:22:21Z

There's a difference in how the image block and Gallery block display their images, with whether the images are stretched to full size:

<figure class="wp-block-image navis-slideshow navis-single navis-full" style="max-width: 100%;">
	<span class="navis-before">X</span>
	<img
		src="http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM.png"
		alt=""
		class="wp-image-1884"
		srcset="http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM.png 420w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-140x140.png 140w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-336x333.png 336w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-60x60.png 60w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-96x96.png 96w"
	>
	<figcaption>Largo is a responsive WordPress theme framework designed for news publishers. Finely-crafted and expertly-honed by the product and technology team at the Institute for Nonprofit News.</figcaption>
</figure>

<ul class="wp-block-gallery columns-1 is-cropped">
	<li class="blocks-gallery-item">
		<figure class="navis-slideshow navis-single navis-full" style="max-width: 100%;">
			<span class="navis-before">X</span>
			<img
				src="http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM.png"
				alt=""
				data-id="1884"
				data-link="http://largodev.innsandbox.wpengine.com/?attachment_id=1884"
				class="wp-image-1884"
				srcset="http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM.png 420w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-140x140.png 140w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-336x333.png 336w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-60x60.png 60w, http://largodev.innsandbox.wpengine.com/wp-content/uploads/2019/07/Screen-Shot-2019-07-17-at-2.48.12-PM-96x96.png 96w"
			>
			<figcaption>Largo is a responsive WordPress theme framework designed for news publishers. Finely-crafted and expertly-honed by the product and technology team at the Institute for Nonprofit News.</figcaption>
		</figure>
	</li>
</ul>

joshdarby · 2019-07-17T20:23:43Z

There's a difference in how the image block and Gallery block display their images, with whether the images are stretched to full size:

@benlk I think that's a known issue, right? And is it in the scope of #1702 to fix in this PR?

benlk · 2019-07-17T20:40:36Z

I'm not sure it's a known issue; I'll create a ticket for it and let's merge this.

#1745 isn't in this milestone either.

Josh Darby added 5 commits May 15, 2019 13:59

Added box-sizing: border-box; attr to figcaptions and wp-caption-text

c7aacd8

Merge branch '0.5-dev' into 1702-add-box-sizing-attribute-to-gallery-…

fee9fbb

…captions

Added box-sizing attr to one more figcaption

6485a84

Updated css files for figcaption that had box-sizing attr added

c0803a5

Updated changelog for PR #1711

4db23ff

joshdarby requested a review from benlk May 20, 2019 13:50

benlk added this to the 0.6.4 milestone May 22, 2019

benlk added category: gutenberg Relating to general Gutenberg compatibility category: images Issues relating to images category: styles affects lots of styles, requiring visual testing Estimate: < 2 Hours labels May 22, 2019

joshdarby mentioned this pull request Jun 17, 2019

Add box-sizing: border-box to navis slideshow caption CSS #1702

Closed

Merge branch '0.5-dev' into 1702-add-box-sizing-attribute-to-gallery-…

09d04a4

…captions

benlk reviewed Jun 17, 2019

View reviewed changes

less/inc/images.less Outdated Show resolved Hide resolved

Remove .is-cropped selector for box-sizing on figcaption in .wp-block…

df15461

…-gallery; other changes: - make the figcaption no more than 50% of height of container - left align text to match other captions - reduce padding on caption

Josh Darby added 2 commits June 24, 2019 11:51

Merge branch '0.5-dev' into 1702-add-box-sizing-attribute-to-gallery-…

e89398f

…captions

Updated navis slideshow slides to have transparent background,

cb4f244

also updated single image block expanded view caption styling to match the classic image.

Updated changelog.md

baffc8e

pullrequest bot reviewed Jun 28, 2019

View reviewed changes

less/inc/images.less Outdated Show resolved Hide resolved

benlk added 4 commits June 30, 2019 22:15

Add comment explaining where spacing comes from.

41d3bc3

Move block styles for navis to blocks.less

8772a8d

Remove style hiding figcaption on image block in slideshow.

50cb50c

Note for future about media credit positioning

3741a35

Fix caption positioning on aligned image blocks and image blocks

2d5db08

pullrequest bot reviewed Jul 5, 2019

View reviewed changes

benlk added the status: needs research We need to look into this to see what's needed label Jul 10, 2019

joshdarby self-assigned this Jul 10, 2019

benlk added a commit to INN/umbrella-innsandbox that referenced this pull request Jul 15, 2019

Largo-dev to WPBuddy/largo#1711 for testing.

5331675

benlk modified the milestone: 0.6.4 Jul 15, 2019

Merge branch '0.5-dev'

4581339

Merge branch '0.5-dev' into 1702-add-box-sizing-attribute-to-gallery-…

d6c739d

…captions

benlk mentioned this pull request Jul 17, 2019

Image and Gallery blocks display smaller-than-viewport images at different sizes #1753

Open

1 task

benlk merged commit 3557a9a into 0.5-dev Jul 17, 2019

benlk deleted the 1702-add-box-sizing-attribute-to-gallery-captions branch July 17, 2019 20:53

joshdarby mentioned this pull request Jul 17, 2019

Full-sized images display differently in image block vs. gallery block #1756

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `box-sizing: border-box;` to... #1711

Added `box-sizing: border-box;` to... #1711

joshdarby commented May 20, 2019

joshdarby commented May 20, 2019

benlk commented Jun 17, 2019

benlk commented Jun 17, 2019 •

edited

Loading

joshdarby commented Jun 18, 2019

benlk commented Jun 18, 2019 •

edited

Loading

benlk commented Jun 19, 2019

benlk commented Jun 19, 2019 •

edited by joshdarby

Loading

joshdarby commented Jun 24, 2019

benlk commented Jun 24, 2019

joshdarby commented Jun 24, 2019 •

edited

Loading

benlk commented Jun 24, 2019

joshdarby commented Jun 24, 2019

pullrequest bot left a comment

benlk commented Jul 1, 2019 •

edited

Loading

benlk commented Jul 1, 2019

benlk commented Jul 1, 2019

benlk commented Jul 2, 2019

pullrequest bot Jul 5, 2019

pullrequest bot Jul 5, 2019

joshdarby commented Jul 9, 2019

benlk commented Jul 9, 2019

benlk commented Jul 15, 2019

joshdarby commented Jul 17, 2019 •

edited

Loading

benlk commented Jul 17, 2019

benlk commented Jul 17, 2019

joshdarby commented Jul 17, 2019

benlk commented Jul 17, 2019 •

edited

Loading

Added box-sizing: border-box; to... #1711

Added box-sizing: border-box; to... #1711

Conversation

joshdarby commented May 20, 2019

joshdarby commented May 20, 2019

benlk commented Jun 17, 2019

benlk commented Jun 17, 2019 • edited Loading

joshdarby commented Jun 18, 2019

benlk commented Jun 18, 2019 • edited Loading

benlk commented Jun 19, 2019

benlk commented Jun 19, 2019 • edited by joshdarby Loading

joshdarby commented Jun 24, 2019

benlk commented Jun 24, 2019

joshdarby commented Jun 24, 2019 • edited Loading

benlk commented Jun 24, 2019

joshdarby commented Jun 24, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

benlk commented Jul 1, 2019 • edited Loading

benlk commented Jul 1, 2019

benlk commented Jul 1, 2019

benlk commented Jul 2, 2019

pullrequest bot Jul 5, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 5, 2019

Choose a reason for hiding this comment

joshdarby commented Jul 9, 2019

benlk commented Jul 9, 2019

benlk commented Jul 15, 2019

joshdarby commented Jul 17, 2019 • edited Loading

benlk commented Jul 17, 2019

benlk commented Jul 17, 2019

joshdarby commented Jul 17, 2019

benlk commented Jul 17, 2019 • edited Loading

Added `box-sizing: border-box;` to... #1711

Added `box-sizing: border-box;` to... #1711

benlk commented Jun 17, 2019 •

edited

Loading

benlk commented Jun 18, 2019 •

edited

Loading

benlk commented Jun 19, 2019 •

edited by joshdarby

Loading

joshdarby commented Jun 24, 2019 •

edited

Loading

benlk commented Jul 1, 2019 •

edited

Loading

joshdarby commented Jul 17, 2019 •

edited

Loading

benlk commented Jul 17, 2019 •

edited

Loading