Implement customizable transition animation duration setting #2321
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #2321 +/- ##
==========================================
- Coverage 68.85% 68.65% -0.21%
==========================================
Files 90 90
Lines 7612 7643 +31
==========================================
+ Hits 5241 5247 +6
- Misses 2371 2396 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Comment from @sarthak-19 in Slack:
|
| ?> | ||
| <style> | ||
| @view-transition { navigation: auto; } | ||
| ::view-transition-group(*) { animation-duration: <?php echo esc_html( (string) $duration_seconds ); ?>s; } |
There was a problem hiding this comment.
esc_attr is probably more appropriate here
There was a problem hiding this comment.
Actually, neither is appropriate, as I've come to find. The contents of STYLE are the same as SCRIPT in that they are “raw text”. Entities are not processed.
In any case, since the value is an integer anyway, we can ensure proper escaping via:
| ::view-transition-group(*) { animation-duration: <?php echo esc_html( (string) $duration_seconds ); ?>s; } | |
| ::view-transition-group(*) { animation-duration: <?php echo (int) $duration_seconds; ?>s; } |
There was a problem hiding this comment.
Right, that is best. esc_html felt too broad, the int cast is perfect.
| ?> | ||
| <input | ||
| type="number" | ||
| id="<?php echo $field_id; ?>" |
There was a problem hiding this comment.
| id="<?php echo $field_id; ?>" | |
| id="<?php echo esc_attr( $args['label_for'] ); ?>" |
| $seconds = (int) $value / 1000; | ||
| $field_id = esc_attr( $args['label_for'] ); |
There was a problem hiding this comment.
Escaping should always be done at the place of printing.
| $seconds = (int) $value / 1000; | |
| $field_id = esc_attr( $args['label_for'] ); | |
| $seconds = (int) $value / 1000; |
| printf( | ||
| /* translators: %s: duration in seconds wrapped in a span for dynamic update */ | ||
| esc_html__( 'ms (%s)', 'view-transitions' ), | ||
| '<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>' |
There was a problem hiding this comment.
| '<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>' | |
| '<span id="' . esc_attr( $args['label_for'] . '-seconds' ) . '">' . esc_html( (string) $seconds ) . 's</span>' |
| /* translators: %s: duration in seconds wrapped in a span for dynamic update */ | ||
| esc_html__( 'ms (%s)', 'view-transitions' ), | ||
| '<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>' | ||
| ); |
There was a problem hiding this comment.
I'm not sure we need the seconds? Can't we just have the milliseconds?
| wp_print_inline_script_tag( | ||
| sprintf( | ||
| 'document.getElementById( %s ).addEventListener( "input", function( e ) { document.getElementById( %s ).textContent = ( parseInt( e.target.value, 10 ) / 1000 ) + "s"; } );', | ||
| wp_json_encode( $field_id ), | ||
| wp_json_encode( $field_id . '-seconds' ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Per above, I don't think we need this.
| wp_print_inline_script_tag( | |
| sprintf( | |
| 'document.getElementById( %s ).addEventListener( "input", function( e ) { document.getElementById( %s ).textContent = ( parseInt( e.target.value, 10 ) / 1000 ) + "s"; } );', | |
| wp_json_encode( $field_id ), | |
| wp_json_encode( $field_id . '-seconds' ) | |
| ) | |
| ); |
| } elseif ( 'number' === $type ) { | ||
| $seconds = (int) $value / 1000; |
There was a problem hiding this comment.
This is a general field output for number but it presumes the number value is going to be seconds or time. That doesn't seem ideal. Note how the checkbox condition above is agnostic to what the setting is for.
| min="<?php echo esc_attr( (string) PLVT_MIN_ANIMATION_DURATION ); ?>" | ||
| max="<?php echo esc_attr( (string) PLVT_MAX_ANIMATION_DURATION ); ?>" |
There was a problem hiding this comment.
These should come from the $args
Part of a polishing issue : #2317
Summary
This PR implements the transition animation duration setting that was previously added to the UI but wasn't functional. The duration setting now properly affects view transitions on both the frontend and WordPress admin area.