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

Jump to bottom

Fix carousel buttons #34266

Merged
merged 2 commits into from Jun 16, 2021
Merged

Fix carousel buttons #34266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a70448
test(carousel): add test to check if next/prev button work as intended
alpadev Jun 16, 2021
b5485c6
fix(carousel): merge passed config with instance config in carouselIn…
alpadev Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion js/src/carousel.js
View file
Open in desktop
Expand Up @@ -498,7 +498,14 @@ class Carousel extends BaseComponent {
static carouselInterface(element, config) {
const data = Carousel.getOrCreateInstance(element, config)

const { _config } = data
let { _config } = data
if (typeof config === 'object') {
_config = {
..._config,
...config
}
}

Copy link
Member

Choose a reason for hiding this comment

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

@alpa Glad you find it ❤️

Can you explain why did this happened?

As it passes from getOrCreateInstance

 static getOrCreateInstance(element, config = {}) {
    return this.getInstance(element) || new this(element, typeof config === 'object' ? config : null)
  }

I am not sure I understand where it fails :/

and _getConfig does the same process

  _getConfig(config) {
    config = {
      ...Default,
      ...Manipulator.getDataAttributes(this._element),
      ...(typeof config === 'object' ? config : {})
    }
    typeCheckConfig(NAME, config, DefaultType)
    return config
  }

const action = typeof config === 'string' ? config : _config.slide

if (typeof config === 'number') {
Expand Down
28 changes: 28 additions & 0 deletions js/tests/unit/carousel.spec.js
View file
Open in desktop
Expand Up @@ -707,6 +707,34 @@ describe('Carousel', () => {

carousel.next()
})

it('should call next()/prev() instance methods when clicking the respective direction buttons', () => {
fixtureEl.innerHTML = [
'<div id="carousel" class="carousel slide">',
' <div class="carousel-inner">',
' <div class="carousel-item active">item 1</div>',
' <div class="carousel-item">item 2</div>',
' <div class="carousel-item">item 3</div>',
' </div>',
' <button class="carousel-control-prev" type="button" data-bs-target="#carousel" data-bs-slide="prev"></button>',
' <button class="carousel-control-next" type="button" data-bs-target="#carousel" data-bs-slide="next"></button>',
'</div>'
].join('')

const carouselEl = fixtureEl.querySelector('#carousel')
const prevBtnEl = fixtureEl.querySelector('.carousel-control-prev')
const nextBtnEl = fixtureEl.querySelector('.carousel-control-next')

const carousel = new Carousel(carouselEl)
const nextSpy = spyOn(carousel, 'next')
const prevSpy = spyOn(carousel, 'prev')

nextBtnEl.click()
prevBtnEl.click()

expect(nextSpy).toHaveBeenCalled()
expect(prevSpy).toHaveBeenCalled()
})
})

describe('nextWhenVisible', () => {
Expand Down