Dailymotion Shortcode: small changes to #4103#1
Conversation
Width and height values should always be full, positive numbers.
It allows third-parties to filter the available shortcode parameters later on.
- add_query_arg builds a cleaner URL, with all necessary appended query variables. - add more checks before a variable is used.
- When we have that value and no width value was defined, it should be the priority.
| if ( ! $width && ! $height ) { | ||
| if ( ! empty( $content_width ) ) { | ||
| $width = min( 425, intval( $content_width ) ); | ||
| $width = absint( $content_width ); |
There was a problem hiding this comment.
hi, I just have a comment here, the old code used to have $width = min( 425, intval( $content_width ) );, this is why I left if, for compatibility reasons. I'm not against using the value when set, but aren't we risking to break the existing usages of this shortcode if we change this?
There was a problem hiding this comment.
It will change things a bit, yes, but I think it's best if $content_width takes priority when it's provided by the theme and when no width is provided in the shortcode.
There was a problem hiding this comment.
alright! I can merge the PR then
|
I just have a comment on one line (about width value), otherwise it's all good for me. When we've clarified this thing about width value, I'll happily merge the PR. |
|
I replied to your comment here: #1 (comment) |
After The Deadline: add spaces for WP coding standards.
There are a few things that I can think could be changed in the PR you submitted here:
Automattic#4103
I think the changes attached to this PR could help.
Let me know if you like them, and if you do, you can merge this PR and all commits will be automatically included in the original PR.