fix(ui5-carousel): enhance public api #3360

kskondov · 2021-05-31T06:13:49Z

Part of #3107

New features:
Added "visibleItemsIndices" public getter returning an array
Added "navigateTo" method that navigates to an item

BREAKING_CHANGE:

selectedIndex property is deprecated
infiniteScrollOffset property is deprecated
load-more event is deprecated

fifoosid · 2021-05-31T07:40:47Z

packages/main/src/Carousel.js

@@ -95,28 +95,16 @@ const metadata = {
 		},

 		/**
-		 * Defines the index of the initially selected item.
+		 * Defines the page of the initially selected item.


Defines the initially selected page

fifoosid · 2021-05-31T07:46:49Z

packages/main/src/Carousel.js

 		 * @type {Integer}
 		 * @defaultvalue 0
 		 * @public
 		 */
-		selectedIndex: {
+		selectedPage: {
 			type: Integer,
 			defaultValue: 0,


I think it would be a good idea to change the defaultValue to 1 and use indexes, starting from 1 rather than 0.

The current behaviour is a little strange. If I want to show the 5th page, I have to say carousel.selectedPage = 4

If this property is private let's call it "_selectedIndex". Also I prefer 0-based indexing to be conistent with HTMLCollection, but we can discuss this

The infiniteScrollOffset property and load-more event and visibleItemsIndices was added to the navigate event, selectedIndex was renamed ti selectedPage

The infiniteScrollOffset property and load-more event were removed, visibleItemsIndices was added to the navigate event, selectedIndex was renamed to selectedPage carousel-api-enhancments

fifoosid

navigateTo method should be implemented as well
visibleItemsIndexes public readonly getter returning an array

Link to the decisions from yesterday

fifoosid · 2021-06-01T07:12:39Z

packages/main/src/Carousel.js

 		 */
-		infiniteScrollOffset: {
+		selectedPage: {


add since tag

fifoosid · 2021-06-01T07:12:57Z

packages/main/src/Carousel.js

@@ -181,29 +169,22 @@ const metadata = {
 	events: /** @lends sap.ui.webcomponents.main.Carousel.prototype */ {

 		/**
-		 * Fired whenever the <code>selectedIndex</code> changes due to user interaction,
+		 * Fired whenever the <code>selectedPage</code> changes due to user interaction,


Fired whenever the selectedPage property changes ...

fifoosid · 2021-06-01T07:16:51Z

packages/main/src/Carousel.js

 		 */
-		infiniteScrollOffset: {
+		selectedPage: {
 			type: Integer,
 			defaultValue: 1,


Yesterday we agreed that the selectedPage should start from 0

fifoosid · 2021-06-01T07:17:03Z

packages/main/src/Carousel.js


-		if (this.selectedIndex - 1 < 0) {
+		if (this.selectedPage - 1 < 1) {


fifoosid · 2021-06-01T07:18:53Z

packages/main/src/Carousel.js

-
-		/**
-		 * Defines when the <code>load-more</code> event is fired. If not applied the event will not be fired.
+		 * Defines the initially selected page.


selectedIndex should remain as private property. This should be added as BREAKING_CHANGE in the description of the PR

fifoosid · 2021-06-01T07:20:38Z

packages/main/src/Carousel.js

 		 * when the user clicks on the navigation arrows or while resizing,
 		 * based on the <code>items-per-page-l</code>, <code>items-per-page-m</code> and <code>items-per-page-s</code> properties.
 		 *
 		 * @event
-		 * @param {Integer} selectedIndex the current <code>selectedIndex</code>.
+		 * @param {Integer} selectedPage the current <code>selectedPage</code>.
+		 * @param {Array} visibleItemsIndexes the indices of the visible items <code>selectedPage</code>.


This should be a readonly getter, rather than an event parameter

…el-api-enhancments

BREAKING_CHANGE Added "visibleItemsIndices" public getter returning an array Added "navigateTo" method that navigates to an item Made "selectedIndex" private

packages/main/src/Carousel.js

dimovpetar · 2021-06-08T14:08:03Z

packages/main/src/Carousel.js

 		 * @type {Integer}
 		 * @defaultvalue 0
 		 * @public
 		 */
-		selectedIndex: {
+		selectedPage: {
 			type: Integer,
 			defaultValue: 0,


If this property is private let's call it "_selectedIndex". Also I prefer 0-based indexing to be conistent with HTMLCollection, but we can discuss this

…ments

georgimkv · 2021-06-15T13:50:47Z

Removed the uses of the selectedIndex attribute from the outside. One test is no longer relevant.
Kept the 0-based counting of pages.

Done

kskondov added for review BREAKING CHANGE labels May 31, 2021

kskondov requested review from ilhan007 and a team May 31, 2021 06:13

fifoosid changed the title ~~Carousel api enhancements~~ fix(ui5-carousel): enhance public api May 31, 2021

fifoosid suggested changes May 31, 2021

View reviewed changes

kskondov added 3 commits May 31, 2021 11:33

refactor(ui5-carousel): API enhancements

7cb0ce3

The infiniteScrollOffset property and load-more event and visibleItemsIndices was added to the navigate event, selectedIndex was renamed ti selectedPage

refactor(ui5-carousel): API enhancements

b10bcb7

The infiniteScrollOffset property and load-more event were removed, visibleItemsIndices was added to the navigate event, selectedIndex was renamed to selectedPage carousel-api-enhancments

Fix aslant issue

e97bbee

kskondov force-pushed the carousel-api-enhancments branch from 8fbffe9 to e97bbee Compare May 31, 2021 08:33

kskondov added 2 commits May 31, 2021 14:52

defaultValue of selectedPage set to 1

24f816f

Fix lint issues

89908d7

fifoosid previously requested changes Jun 1, 2021

View reviewed changes

kskondov and others added 4 commits June 3, 2021 16:24

Merge branch 'master' of github.com:SAP/ui5-webcomponents into carous…

215012a

…el-api-enhancments

fix(ui5-carousel): enhance public api

650250a

BREAKING_CHANGE Added "visibleItemsIndices" public getter returning an array Added "navigateTo" method that navigates to an item Made "selectedIndex" private

add jsdoc

17f4c18

Merge branch 'master' into carousel-api-enhancments

ea236c7

georgimkv requested a review from fifoosid June 7, 2021 09:33

Merge branch 'master' into carousel-api-enhancments

bba2641

dimovpetar reviewed Jun 8, 2021

View reviewed changes

georgimkv added 2 commits June 15, 2021 16:21

Merge remote-tracking branch 'origin/master' into carousel-api-enhanc…

a9d463e

…ments

rename selectedIndex and removed public feature

c7fc705

georgimkv requested a review from dimovpetar June 15, 2021 13:51

remove navigate event firing when navigateTo gets called

848fd43

dimovpetar approved these changes Jun 16, 2021

View reviewed changes

georgimkv closed this Jun 18, 2021

georgimkv reopened this Jun 18, 2021

fifoosid approved these changes Jun 29, 2021

View reviewed changes

fifoosid merged commit 351d289 into master Jun 29, 2021

fifoosid deleted the carousel-api-enhancments branch June 29, 2021 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ui5-carousel): enhance public api #3360

fix(ui5-carousel): enhance public api #3360

kskondov commented May 31, 2021 •

edited by fifoosid

Loading

fifoosid May 31, 2021

fifoosid May 31, 2021

dimovpetar Jun 8, 2021

fifoosid left a comment

fifoosid Jun 1, 2021

fifoosid Jun 1, 2021

fifoosid Jun 1, 2021

fifoosid Jun 1, 2021

fifoosid Jun 1, 2021

fifoosid Jun 1, 2021

dimovpetar Jun 8, 2021

georgimkv commented Jun 15, 2021


		if (this.selectedIndex - 1 < 0) {
		if (this.selectedPage - 1 < 1) {

fix(ui5-carousel): enhance public api #3360

fix(ui5-carousel): enhance public api #3360

Conversation

kskondov commented May 31, 2021 • edited by fifoosid Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fifoosid left a 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

georgimkv commented Jun 15, 2021

kskondov commented May 31, 2021 •

edited by fifoosid

Loading