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.

Sign up for GitHub

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

Fix carousel transition duration #25218

Merged
merged 21 commits into from
Feb 19, 2018
Merged

Fix carousel transition duration #25218

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e84f92c
Fix carousel transition duration
MartijnCuppens Jan 6, 2018
0a4967c
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 8, 2018
79b5335
Add visual test for changed transition-duration of carousel-item
MartijnCuppens Jan 9, 2018
1e13251
Merge branch 'v4-dev' into fix-carousel-transition-duration
Johann-S Jan 11, 2018
44fb2e9
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 11, 2018
1206b54
Carousel: transition duration documentation
MartijnCuppens Jan 11, 2018
bbc2d66
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 11, 2018
c385f68
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 12, 2018
86d3645
Remove newline
MartijnCuppens Jan 14, 2018
7ba6e72
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 14, 2018
11460c4
Merge branch 'v4-dev' into fix-carousel-transition-duration
XhmikosR Jan 15, 2018
eeef19b
Update carousel.md
XhmikosR Jan 15, 2018
c6b721a
Break line.
XhmikosR Jan 15, 2018
cb2873a
adjust docs copy
mdo Jan 16, 2018
39ca5be
shorter heading
mdo Jan 16, 2018
7a4cc99
formatting of comment
mdo Jan 16, 2018
5527e9c
Remove ES6
MartijnCuppens Jan 16, 2018
ce25310
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 16, 2018
463db01
Merge branch 'v4-dev' into fix-carousel-transition-duration
MartijnCuppens Jan 20, 2018
3ff95f6
Merge branch 'v4-dev' into fix-carousel-transition-duration
XhmikosR Feb 1, 2018
fd9e5d4
Merge branch 'v4-dev' into fix-carousel-transition-duration
Johann-S Feb 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 39 additions & 20 deletions js/src/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@ const Carousel = (($) => {
* ------------------------------------------------------------------------
*/

const NAME = 'carousel'
const VERSION = '4.0.0-beta.3'
const DATA_KEY = 'bs.carousel'
const EVENT_KEY = `.${DATA_KEY}`
const DATA_API_KEY = '.data-api'
const JQUERY_NO_CONFLICT = $.fn[NAME]
const TRANSITION_DURATION = 600
const ARROW_LEFT_KEYCODE = 37 // KeyboardEvent.which value for left arrow key
const ARROW_RIGHT_KEYCODE = 39 // KeyboardEvent.which value for right arrow key
const TOUCHEVENT_COMPAT_WAIT = 500 // Time for mouse compat events to fire after touch
const NAME = 'carousel'
const VERSION = '4.0.0-beta.3'
const DATA_KEY = 'bs.carousel'
const EVENT_KEY = `.${DATA_KEY}`
const DATA_API_KEY = '.data-api'
const JQUERY_NO_CONFLICT = $.fn[NAME]
const ARROW_LEFT_KEYCODE = 37 // KeyboardEvent.which value for left arrow key
const ARROW_RIGHT_KEYCODE = 39 // KeyboardEvent.which value for right arrow key
const TOUCHEVENT_COMPAT_WAIT = 500 // Time for mouse compat events to fire after touch
const MILLISECONDS_MULTIPLIER = 1000

const Default = {
interval : 5000,
Expand Down Expand Up @@ -94,18 +94,20 @@ const Carousel = (($) => {
class Carousel {

constructor(element, config) {
this._items = null
this._interval = null
this._activeElement = null
this._items = null
this._interval = null
this._activeElement = null

this._isPaused = false
this._isSliding = false
this._isPaused = false
this._isSliding = false

this.touchTimeout = null
this.touchTimeout = null

this._config = this._getConfig(config)
this._element = $(element)[0]
this._indicatorsElement = $(this._element).find(Selector.INDICATORS)[0]
this._config = this._getConfig(config)
this._element = $(element)[0]
this._indicatorsElement = $(this._element).find(Selector.INDICATORS)[0]

this._transitionDuration = this._getTransitionDuration()

this._addEventListeners()
}
Expand Down Expand Up @@ -231,6 +233,23 @@ const Carousel = (($) => {
return config
}

_getTransitionDuration() {
// Get transition-duration of first element in the carousel
let transitionDuration = $(this._element).find(Selector.ITEM).css('transition-duration')

// Return 0 carousel item is not found
if (!transitionDuration) {
return 0
}

// If multiple durations are defined, take the first
transitionDuration = transitionDuration.split(',')[0]

// Multiply by 1000 if transition-duration is defined in seconds
return transitionDuration.indexOf('ms') > -1 ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use includes here, does babel take care of it or do we need a polyfill?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even endsWith?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XhmikosR, just tested this, includes and endsWith are not polyfilled by babel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing. Not sure if we need to enable something else in our babel config, but it gets out of this PRs scope anyway.

parseFloat(transitionDuration) : parseFloat(transitionDuration) * MILLISECONDS_MULTIPLIER
}

_addEventListeners() {
if (this._config.keyboard) {
$(this._element)
Expand Down Expand Up @@ -410,7 +429,7 @@ const Carousel = (($) => {
setTimeout(() => $(this._element).trigger(slidEvent), 0)

})
.emulateTransitionEnd(TRANSITION_DURATION)
.emulateTransitionEnd(this._transitionDuration)

} else {
$(activeElement).removeClass(ClassName.ACTIVE)
Expand Down
14 changes: 12 additions & 2 deletions js/tests/visual/carousel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,17 @@
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<link rel="stylesheet" href="../../../dist/css/bootstrap.min.css">
<title>Carousel</title>
<style>
.carousel-item {
transition: transform 2s ease, opacity .5s ease;
}
</style>
</head>
<body>
<div class="container">
<h1>Carousel <small>Bootstrap Visual Test</small></h1>

<p>Also, the carousel shouldn't slide when its window/tab is hidden. Check the console log.</p>
<p>The transition duration should be around 2s. Also, the carousel shouldn't slide when its window/tab is hidden. Check the console log.</p>

<div id="carousel-example-generic" class="carousel slide" data-ride="carousel">
<ol class="carousel-indicators">
Expand Down Expand Up @@ -46,9 +51,14 @@ <h1>Carousel <small>Bootstrap Visual Test</small></h1>

<script>
$(function() {
var t0, t1;

// Test to show that the carousel doesn't slide when the current tab isn't visible
$('#carousel-example-generic').on('slid.bs.carousel', function(event) {
console.log('slid at ', event.timeStamp)
t1 = performance.now()
console.log(`transition-duration took ${t1 - t0}ms, slid at `, event.timeStamp)
}).on('slide.bs.carousel', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the use of ES6 template here, we aren't in our code source

t0 = performance.now()
})
})
</script>
Expand Down
3 changes: 2 additions & 1 deletion scss/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,8 @@ $carousel-control-icon-width: 20px !default;

$carousel-control-prev-icon-bg: str-replace(url("data:image/svg+xml;charset=utf8,%3Csvg xmlns='http://www.w3.org/2000/svg' fill='#{$carousel-control-color}' viewBox='0 0 8 8'%3E%3Cpath d='M5.25 0l-4 4 4 4 1.5-1.5-2.5-2.5 2.5-2.5-1.5-1.5z'/%3E%3C/svg%3E"), "#", "%23") !default;
$carousel-control-next-icon-bg: str-replace(url("data:image/svg+xml;charset=utf8,%3Csvg xmlns='http://www.w3.org/2000/svg' fill='#{$carousel-control-color}' viewBox='0 0 8 8'%3E%3Cpath d='M2.75 0l-1.5 1.5 2.5 2.5-2.5 2.5 1.5 1.5 4-4-4-4z'/%3E%3C/svg%3E"), "#", "%23") !default;

// If multiple transitions are applied, make sure the transform transition is defined first
// eg. transform 2s ease, opacity .5s ease-out
$carousel-transition: transform .6s ease !default;


Expand Down