-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
OEL-1912: Revamp carousel pattern. #265
Conversation
label: 'Show indicators' | ||
description: 'With indicators (default: false).' | ||
preview: true | ||
autoplay: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we could keep this one, what do you think ?
'items': _slides, | ||
'fade': fade, | ||
'with_controls': show_controls, | ||
'with_controls': true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there are defaults, but want to set in stone the behaviour
@@ -1,54 +1,19 @@ | |||
carousel: | |||
label: 'Carousel' | |||
description: 'A slideshow component for cycling through elements, like a carousel. For more information: https://v5.getbootstrap.com/docs/5.0/components/carousel/' | |||
settings: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we shipped with these field already, so we cannot remove them anymore. I didn't realise for some reason that we had already a pattern when I did the analysis. We need to leave it untouched in this version.
*/ | ||
protected function assertItems(array $expected, Crawler $crawler): void { | ||
$items = $crawler->filter('.carousel .carousel-inner .carousel-item'); | ||
self::assertCount(count($expected), $items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self::assertCount(count($expected), $items); | |
self::assertSameSize($expected, $items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
} | ||
|
||
if (!empty($expected['show_controls'])) { | ||
$this->assertElementText('Previous', '.carousel-control-prev', $crawler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these methods can receive NULL and they will check for absence of element/attribute.
We could rewrite (some of) them like this:
$this->assertElementText('Previous', '.carousel-control-prev', $crawler); | |
$this->assertElementText($expected['show_controls'] ?? NULL, '.carousel-control-prev', $crawler); |
No description provided.