Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#30201 closed enhancement (wontfix)

img_caption_shortcode_width should support percentage widths

Reported by: danielbachhuber's profile danielbachhuber Owned by: polevaultweb's profile polevaultweb
Milestone: Priority: normal
Severity: normal Version:
Component: Media Keywords: good-first-bug has-patch needs-testing close
Focuses: Cc:

Description

Currently, it assumes the returned value to be pixels.

Attachments (1)

30201.patch (1.2 KB) - added by polevaultweb 9 years ago.
This patch adds a right margin of 10px when a percentage width is returned from the filter. This is my first patch/contribution/trac usage so go easy :)

Download all attachments as: .zip

Change History (17)

#1 @hiwhatsup
9 years ago

Maybe something like this might work? Thoughts?

	if ( strpos ( $atts['width'], '%' ) !== false ) {
		$caption_width_um = "%";
	} else {
		$caption_width_um = "px";
	}
	$atts['width'] = (int) $atts['width'];
	if ( current_theme_supports( 'html5', 'caption' ) ) {
		return '<figure ' . $atts['id'] . 'style="width: ' . (int) $atts['width'] . $caption_width_um .  ';" class="' . esc_attr( $class ) . '">'
		. do_shortcode( $content ) . '<figcaption class="wp-caption-text">' . $atts['caption'] . '</figcaption></figure>';
	}

Since by default, the caption is 10 pixels greater than the width of the image. We'll need to add those 10px using CSS3 function calc(). I'm not how well this will work on older browsers. Another option is to add a margin or padding.

	if ( $caption_width ) {
		if ($caption_width_um == "%") {
			$style = 'style="width: calc('. (int) $caption_width .'% + 10px); width: -moz-calc('. (int) $caption_width .'% + 10px); -webkit-calc('. (int) $caption_width .'% + 10px);  -o-calc('. (int) $caption_width .'% + 10px); " ';
			}
		else {
			$style = 'style="width: ' . (int) $caption_width . 'px" ';
			}
		

	return '<div ' . $atts['id'] . $style . 'class="' . esc_attr( $class ) . '">'
	. do_shortcode( $content ) . '<p class="wp-caption-text">' . $atts['caption'] . '</p></div>';
	}

This is my first time here after attending @wcsf14. I wanted to ask before committing a diff and get some feedback for veteran committers and contributors.

Last edited 9 years ago by hiwhatsup (previous) (diff)

#2 @williampatton
9 years ago

I'm not exactly a veteran contributor or anything but I'm not certain that using a css3 function is the best idea for this as there's still a fair amount of people stuck using browsers that don't support css3 at all. Worried it wouldn't degrade gracefully for those users.

I prefer your suggestion of using margin/padding instead as the solution to add the extra space.

#3 @Yahire Furniture
9 years ago

Would It not be easier to just keep the style of the caption to within the css?

#4 @williampatton
9 years ago

Not possible to keep it purely within the css (I assume you mean a stylesheet) as it needs to output a user specified value.

Last edited 9 years ago by williampatton (previous) (diff)

@polevaultweb
9 years ago

This patch adds a right margin of 10px when a percentage width is returned from the filter. This is my first patch/contribution/trac usage so go easy :)

#5 @polevaultweb
9 years ago

  • Keywords has-patch added; needs-patch removed

#6 @polevaultweb
9 years ago

  • Keywords needs-testing added

#7 @kevdotbadger
9 years ago

  • Keywords dev-feedback added

Isn't the margins added automatically when the alignleft and alignright classes are added? @hiwhatsup's solution seems to work nicely, however tinyMCE strips away the % from the shortcode, so that needs patching as well. the current regex is /width=['"]([0-9]*)['"] ?/ which matches just numbers, we need to amend that so that it matches numbers as well as %.

I think a new attr might be the best way, a 'caption_width_unit' one which defaults to px for backwards compatibility, and allows either blank (as default), 'px' or '%'. This way we could also add em or whatever later.

Thoughts?

#8 @azaozz
9 years ago

How would the percentage width be used? Limiting with max-width (in combination with @media) seems to cover nearly all user cases?

Also, percentage width on a big screen might stretch it over the image size.

#9 follow-up: @danielbachhuber
9 years ago

IIRC, we were doing something with 50% width (and float left / right). We used media queries to set lower and upper bounds.

#10 in reply to: ↑ 9 ; follow-ups: @azaozz
9 years ago

  • Keywords close added; dev-feedback removed

Replying to danielbachhuber:

There is no good way to use default percentage width without also setting min and max width. It will also force scaling of the image, possibly up-scaling in some cases.

Thinking this is best left for the theme to implement. I know it may need an !important rule, but probably warranted here. Suggesting wontfix.

Last edited 9 years ago by azaozz (previous) (diff)

#11 in reply to: ↑ 10 ; follow-up: @williampatton
9 years ago

Replying to azaozz:

I don't think that anyone is suggesting a default percentage width - more that they would like the option of setting a percentage if the usage called for it. The default should still be pixel based as defined by the theme image sizes.

Also I agree that percentage widths don't have many good usage cases unless the theme specifies a min-width/max-width but don't think we should totally disregard this idea because of that. That being said your the one here with most experience of developing WP so you're probably the one in the best position to make that decision :p

Last edited 9 years ago by williampatton (previous) (diff)

#12 in reply to: ↑ 11 ; follow-up: @azaozz
9 years ago

Replying to williampatton:

I don't think that anyone is suggesting a default percentage width...

I meant supporting a percentage width for image captions as an option in TinyMCE, sorry for not making it clearer. It's not acceptable to have it work one way in the editor and another way on the front-end. Currently this is possible to do both in the editor and on the front-end with width: 50% !important, etc. and IMHO this covers the user cases well.

#13 in reply to: ↑ 12 @williampatton
9 years ago

Replying to azaozz:

I meant supporting a percentage width for image captions as an option in TinyMCE, sorry for not making it clearer. It's not acceptable to have it work one way in the editor and another way on the front-end. Currently this is possible to do both in the editor and on the front-end with width: 50% !important, etc. and IMHO this covers the user cases well.

Ahh OK I get you. To be honest I hadn't realized there would be a discrepancy between editor and front-end. Being someone who's more comfortable using the text editor than the visual one I often overlook issues like that.

There are a handful of cases where allowing a percentage to be used would be handy but I'm starting to see your point and edging more towards your wontfix suggestion. Themes that set something like width:50% !important definitely would cover most use cases - and since [caption] can accept a custom class name I think that offers an acceptable way to set anything else that might be required.

I second the notion of 'wontfix' for this ticket but can't help thinking that there's got to be something that could be done for anyone that does need to be specify percentages for caption sizes... unfortunately I'm not really sure what that 'something that could be done' might be :(

#14 @DrewAPicture
9 years ago

  • Owner set to polevaultweb
  • Status changed from new to assigned

#15 in reply to: ↑ 10 @polevaultweb
9 years ago

Replying to azaozz:

Thinking this is best left for the theme to implement. I know it may need an !important rule, but probably warranted here. Suggesting wontfix.

Ok so I have been assigned this ticket, but are we thinking its a wontfix? Happy to defer to your judgement on this @azaozz

#16 @wonderboymusic
9 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

as suggested by the owner

Note: See TracTickets for help on using tickets.