feat: (Core) introduce new Carousel component #3551

DeepakSap14 · 2020-10-05T09:59:52Z

Please provide a link to the associated issue.

#3279

Please provide a brief summary of this pull request.

Introducing carousel component in core.

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/master/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist

Documentation checklist:

update README.md
Documentation Examples
Stackblitz works for all examples

Known issues:

Looping is not working for Multiple active item.
Mobile swipe functionality not implemented.

netlify · 2020-10-05T10:07:11Z

Deploy preview for fundamental-ngx ready!

Built with commit 40dad4c

https://deploy-preview-3551--fundamental-ngx.netlify.app

sKudum · 2020-10-05T14:02:56Z

libs/core/src/lib/carousel/carousel.component.ts

+})
+export class CarouselComponent implements OnInit, AfterContentInit, OnDestroy {
+    /** @hidden */
+    @ContentChildren(CarouselItemComponent)


we need to follow angular coding guidelines for properties alignment. reference angular.io, we can update where ever it is applicable

fixed. followed angular guidelines.

sKudum · 2020-10-05T14:03:28Z

libs/core/src/lib/carousel/carousel.component.ts

+
+    /** Id for the Carousel. */
+    @Input()
+    @HostBinding('attr.id')


It is recommended not to use @HostBinding() along with @input()

I see its getting used in core components. Do you have any other approach?

IMO IT's correct

sKudum · 2020-10-05T14:04:58Z

libs/core/src/lib/carousel/carousel-item/carousel-item.component.ts

+    changeDetection: ChangeDetectionStrategy.OnPush,
+    encapsulation: ViewEncapsulation.None
+})
+export class CarouselItemComponent {


Base component extension won't help here?

Base component is in platform. carousel is in core. so will not be able to use base component.

sKudum · 2020-10-05T14:05:50Z

libs/core/src/lib/carousel/carousel-item/carousel-item.component.spec.ts

+  it('should create', () => {
+    expect(component).toBeTruthy();
+  });
+});


having few test scenarios will help.

have included tests in carousel component for carousel items as well. removed this file.

sKudum · 2020-10-05T14:07:57Z

libs/core/src/lib/carousel/carousel.module.ts

+    imports: [CommonModule],
+    exports: [CarouselComponent, CarouselItemComponent]
+})
+export class CarouselNewModule {}


Only core we have a carousel why should we have "New"

using CarouselModule now.

mikerodonnell89 · 2020-10-05T17:18:59Z

The images in the preview take a very long time to load if they're not cached, perhaps each example can share a service that will load the images so they'll only need to be downloaded once. Also I think that service itself is just very slow so maybe use a different service, placeimg seems to be faster

mikerodonnell89 · 2020-10-05T17:22:35Z

Thumb swiping for the mobile example (hidden navigation buttons) does not work. This does work for cozy sized multiinputs, check the css

mikerodonnell89 · 2020-10-05T18:33:57Z

Playing with the width/height inputs, the carousel items overflow beyond the boundaries, is that intentional?

mikerodonnell89 · 2020-10-05T18:39:52Z

I think <div style="display: flex; justify-content: center; margin-top: 0px;"> can be built in to the component rather than requiring the dev to add it themselves

droshev

have you implemented the sliding effect:

When the user navigates from the current item to another item, the current item is moved out of the content area, and the next or previous item slides in (depending on the direction of navigation).

On touch devices, users navigate with swipe gestures (swipe right or swipe left).

also the animation is missing when you change the component in

droshev · 2020-10-05T23:47:19Z

I think <div style="display: flex; justify-content: center; margin-top: 0px;"> can be built in to the component rather than requiring the dev to add it themselves

I agree with @mikerodonnell89 and also it doesn't follow the markup from styles

droshev · 2020-10-05T23:48:09Z

ork for coz

the sliding should work on any device including the browser

kavya-b · 2020-10-06T10:12:54Z

...ocs/src/app/core/component-docs/carousel/examples/carousel-content-navigation.component.html

@@ -0,0 +1,39 @@
+<fd-carousel [navigatorInPageIndicator]="false" [pageIndicatorContainerPlacement]="'top'">


remove "'top'" [] binding here and at other places

kavya-b · 2020-10-06T10:18:07Z

libs/core/src/lib/carousel/carousel.component.ts

+})
+export class CarouselComponent implements OnInit, AfterContentInit, OnDestroy {
+    /** @hidden */
+    @ContentChildren(CarouselItemComponent, { descendants: true })


I think @ContentChildren is supposed to come after Input() and Output(), right?

kavya-b · 2020-10-06T10:18:55Z

libs/core/src/lib/carousel/carousel.component.ts

+const ICON_PAGE_INDICATOR_LIMIT = 8;
+let carouselUniqueId = 0;
+
+class CarouselActiveItem {


would this need to be exported?

fixed. export added

Betrozov · 2020-10-06T10:32:33Z

...docs/src/app/core/component-docs/carousel/examples/carousel-no-page-indicator.component.html

@@ -0,0 +1,35 @@
+<fd-carousel [showPageIndicator]="false" [pageIndicatorContainerPlacement]="'top'">


unnecessary binding

JKMarkowski · 2020-10-06T11:08:20Z

I agree with @droshev, this should be used with some functionality of carousel.directive from utils. There is already implemented swipe support

InnaAtanasova · 2020-10-22T18:13:16Z

Hey @DeepakSap14 ,
I found some minor issues testing the component on a mobile device:

if you slide slowly sometimes two images are visible at the same time. I understand it's a performance issue but if it's not addressed now could you create an issue for targeting this in the future. Here's a screenshot of the issue:
Landscape view, you can't make the last card appear in the view. The right button is disabled and the last "dot" is marked as active but you basically can't display the last item. See screenshot:
this is similar to the issue above. In portrait mode you can't display cards 7th and 8th:
Message page is not centered on a mobile device

DeepakSap14 · 2020-10-23T02:36:09Z

Great work! Few things:

it looks like there is a double border on the bottom part:

there is a weird effect when you try to drag images:

can we achieve this sliding with the mouse from the carousel with multiple items in the carousel with 1 active item?

carousel styles does not define focus. So each browser was behaving differently by default. Now for documentation i have included, fd-carousel:focus styles. It is consistent now.
fixed by pointer-event: none style.
fixed. swipe working for single active item as well.

carousel service

fix: duplicate code removed

unnecessary changes removed error removed

DeepakSap14 · 2020-10-23T11:31:12Z

Hey @DeepakSap14 ,
I found some minor issues testing the component on a mobile device:

if you slide slowly sometimes two images are visible at the same time. I understand it's a performance issue but if it's not addressed now could you create an issue for targeting this in the future. Here's a screenshot of the issue:

Landscape view, you can't make the last card appear in the view. The right button is disabled and the last "dot" is marked as active but you basically can't display the last item. See screenshot:

this is similar to the issue above. In portrait mode you can't display cards 7th and 8th:

Message page is not centered on a mobile device

@InnaAtanasova

I did not get this issue. When animation effect is sliding. then in this current active item slides out and new item slides in. In this process, until sliding is completed. part of both item will be visible. see this example: http://css3.bradshawenterprises.com/sliding/
fixed hidden card issue in Mobile view portrait and landscape.
fixed hidden card issue in Mobile view portrait and landscape.
Error message is center aligned in every screen now.

DeepakSap14 · 2020-10-23T11:32:55Z

@InnaAtanasova @droshev This can be reviewed now.
I will hold this for merge as you had suggested.

InnaAtanasova

@DeepakSap14 I verified that my comments have been addressed. Thanks for doing this. Also, great job with the component.

droshev

looks much better!
One thing i noticed when you Tab over the carousel with multiple items the content gets shifted but the little dots indicating the current page don't.

manavs · 2020-10-27T01:14:36Z

looks much better!
One thing i noticed when you Tab over the carousel with multiple items the content gets shifted but the little dots indicating the current page don't.

May be I am viewing this incorrectly, but looks like when tabbing through to the card which is current is always the middle one. Not sure whether that it part of the requirement spec. This happens when the new set of cards come under view

DeepakSap14 · 2020-10-28T18:08:11Z

looks much better!
One thing i noticed when you Tab over the carousel with multiple items the content gets shifted but the little dots indicating the current page don't.

@droshev
taborder issue has been fixed now.
please review.

droshev

@DeepakSap14 Great work! LGTM 🚀

DeepakSap14 self-assigned this Oct 5, 2020

DeepakSap14 added 1p core Core library specific issues labels Oct 5, 2020

DeepakSap14 added this to In progress in Development via automation Oct 5, 2020

DeepakSap14 linked an issue Oct 5, 2020 that may be closed by this pull request

Carousel component implementation in ngx-core #3279

Closed

DeepakSap14 mentioned this pull request Oct 5, 2020

Carousel component implementation in ngx-core #3279

Closed

InnaAtanasova removed this from In progress in Development Oct 5, 2020

InnaAtanasova changed the title ~~fix: Introduce Core carousel component~~ feat: (Core) introduce new Carousel component Oct 5, 2020

DeepakSap14 requested review from KevinOkamoto, sKudum, Lokanathan-k, kavya-b, manu-kr and fkolar October 5, 2020 13:44

sKudum suggested changes Oct 5, 2020

View reviewed changes

droshev added this to In progress in Development via automation Oct 5, 2020

droshev self-requested a review October 5, 2020 23:44

droshev requested changes Oct 5, 2020

View reviewed changes

kavya-b reviewed Oct 6, 2020

View reviewed changes

Betrozov approved these changes Oct 6, 2020

View reviewed changes

DeepakSap14 added 10 commits October 23, 2020 16:21

refactor

ae244df

fix:added test cases

0277f1b

carousel service

carousel service addeed with rtl

c5af106

fix:swiping, looping working

ec2987f

fix: duplicate code removed

fix:cards used in example

4d289d7

unnecessary changes removed error removed

fix:looped carousel bug

1c3955c

fix:multiple item swipe in one time

db87528

fix:Internet explorer width height handling

cce3ab8

addressed deno's comments

fb82001

AfterViewChecked added

d4dcb2a

DeepakSap14 force-pushed the coreCarouselComponent branch from f75df51 to d4dcb2a Compare October 23, 2020 11:26

DeepakSap14 requested a review from droshev October 23, 2020 13:09

InnaAtanasova self-requested a review October 24, 2020 18:30

InnaAtanasova approved these changes Oct 24, 2020

View reviewed changes

droshev added this to the Sprint 49 - ariba milestone Oct 26, 2020

droshev requested changes Oct 26, 2020

View reviewed changes

manjunathanagaraj mentioned this pull request Oct 28, 2020

Spring 49 - goals #3387

Closed

fix:taborder handling with visibility

40dad4c

DeepakSap14 requested a review from droshev October 28, 2020 18:07

droshev reviewed Oct 29, 2020

View reviewed changes

droshev self-requested a review October 30, 2020 12:31

droshev approved these changes Oct 30, 2020

View reviewed changes

DeepakSap14 merged commit 2c625cb into master Oct 30, 2020

DeepakSap14 deleted the coreCarouselComponent branch October 30, 2020 12:33

		@@ -0,0 +1,39 @@
		<fd-carousel [navigatorInPageIndicator]="false" [pageIndicatorContainerPlacement]="'top'">

		@@ -0,0 +1,35 @@
		<fd-carousel [showPageIndicator]="false" [pageIndicatorContainerPlacement]="'top'">

feat: (Core) introduce new Carousel component #3551

feat: (Core) introduce new Carousel component #3551

Conversation

DeepakSap14 commented Oct 5, 2020 • edited

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Please check whether the PR fulfills the following requirements

netlify bot commented Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikerodonnell89 commented Oct 5, 2020

mikerodonnell89 commented Oct 5, 2020 • edited

mikerodonnell89 commented Oct 5, 2020

mikerodonnell89 commented Oct 5, 2020

droshev left a comment

Choose a reason for hiding this comment

droshev commented Oct 5, 2020

droshev commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JKMarkowski commented Oct 6, 2020

InnaAtanasova commented Oct 22, 2020 • edited by DeepakSap14

DeepakSap14 commented Oct 23, 2020

DeepakSap14 commented Oct 23, 2020

DeepakSap14 commented Oct 23, 2020

InnaAtanasova left a comment

Choose a reason for hiding this comment

droshev left a comment • edited

Choose a reason for hiding this comment

manavs commented Oct 27, 2020

DeepakSap14 commented Oct 28, 2020

droshev left a comment

Choose a reason for hiding this comment

DeepakSap14 commented Oct 5, 2020 •

edited

netlify bot commented Oct 5, 2020 •

edited

mikerodonnell89 commented Oct 5, 2020 •

edited

InnaAtanasova commented Oct 22, 2020 •

edited by DeepakSap14

droshev left a comment •

edited