search

brainhubeu / react-carousel

A pure extendable React carousel, powered by Brainhub (craftsmen who ❤️ JS)
https://brainhub.eu/
MIT License
1.07k stars 164 forks source link

Fix/pr 606 aayush420 master #608

Open piotr-s-brainhub opened 4 years ago

piotr-s-brainhub commented 4 years ago

Deployed to https://beghp.github.io/gh-pages-rc-6
Fixes #298

- [x] an issue linked to the PR

aayush420 commented 4 years ago

Ignore this comment, I've found how to merge the new commit into the new branch. this is covered in #609. Merge it to the new branch instead.

@piotr-s-brainhub Please lint and fix the code to ensure that the CI test case is passed. After linting, following will be the output.

+++ b/src/components/Carousel.js
@@ -578,18 +578,18 @@ class Carousel extends Component {
     const additionalOffset = this.getProp('centered')
       ? (this.state.carouselWidth / 2) - (elementWidthWithOffset / 2)
       : 0;
-    const stickyEdges = this.getProp('stickyEdges')
+    const stickyEdges = this.getProp('stickyEdges');
     const dragOffset = this.getProp('draggable') ? this.state.dragOffset : 0;
     const currentValue = this.getActiveSlideIndex();
     const additionalClonesOffset = this.getAdditionalClonesOffset();
-    const slidesPerScroll = this.getProp('slidesPerScroll')
+    const slidesPerScroll = this.getProp('slidesPerScroll');
     if (stickyEdges) {
-      return dragOffset 
-        - ((currentValue > this.getChildren().length - slidesPerScroll && !this.getProp('infinite')) ? (this.getChildren().length - slidesPerScroll): currentValue) * elementWidthWithOffset 
+      return dragOffset
+        - ((currentValue > this.getChildren().length - slidesPerScroll && !this.getProp('infinite')) ? (this.getChildren().length - slidesPerScroll) : currentValue) * elementWidthWithOffset
         - additionalClonesOffset;
-    }
-    else 
+    } else {
       return dragOffset - currentValue * elementWidthWithOffset + additionalOffset - additionalClonesOffset;
+    }

The same can be found from the commit https://github.com/aayush420/react-carousel/commit/f6e26e4136bcc83832ed0852db7ccd3f921c9e4b . I'm not able add more commits to my existing PR, as it has already been merged. Will make sure to lint and also test the code, before sending a PR, from next time.

aayush420 commented 4 years ago

Hey, I've added https://github.com/brainhubeu/react-carousel/pull/609 to fix the linting issue and thus passes the unit test.

brainhub-devops-2 commented 4 years ago
Warnings
:warning: MODULE : node_modules/uglify-js/node_modules/wordwrap | LICENSE : MIT* | LICENSE_FILE : node_modules/uglify-js/node_modules/wordwrap/README.markdown | REPOSITORY: https://github.com/substack/node-wordwrap | PUBLISHER : James Halliday | EMAIL : mail@substack.net | URL : http://substack.net
:warning: MODULE : node_modules/vargs | LICENSE : MIT* | LICENSE_FILE : node_modules/vargs/LICENSE | REPOSITORY: undefined | PUBLISHER : Alexis Sellier | EMAIL : self@cloudhead.net | URL : undefined
:warning: MODULE : node_modules/gemini-coverage/node_modules/uglify-js | LICENSE : BSD* | LICENSE_FILE : node_modules/gemini-coverage/node_modules/uglify-js/LICENSE | REPOSITORY: https://github.com/mishoo/UglifyJS2 | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/trim | LICENSE : MIT* | LICENSE_FILE : node_modules/trim/Readme.md | REPOSITORY: undefined | PUBLISHER : TJ Holowaychuk | EMAIL : tj@vision-media.ca | URL : undefined
:warning: MODULE : node_modules/istanbul/node_modules/source-map | LICENSE : BSD | LICENSE_FILE : node_modules/istanbul/node_modules/source-map/LICENSE | REPOSITORY: https://github.com/mozilla/source-map | PUBLISHER : Nick Fitzgerald | EMAIL : nfitzgerald@mozilla.com | URL : undefined
:warning: MODULE : node_modules/gemini-coverage/node_modules/source-map | LICENSE : BSD | LICENSE_FILE : node_modules/gemini-coverage/node_modules/source-map/LICENSE | REPOSITORY: https://github.com/mozilla/source-map | PUBLISHER : Nick Fitzgerald | EMAIL : nfitzgerald@mozilla.com | URL : undefined
:warning: MODULE : node_modules/optimist | LICENSE : MIT* | LICENSE_FILE : node_modules/optimist/LICENSE | REPOSITORY: https://github.com/substack/node-optimist | PUBLISHER : James Halliday | EMAIL : mail@substack.net | URL : http://substack.net
:warning: MODULE : node_modules/map-stream | LICENSE : Custom: https://github.com/dominictarr/event-stream | LICENSE_FILE : node_modules/map-stream/LICENCE | REPOSITORY: https://github.com/dominictarr/map-stream | PUBLISHER : Dominic Tarr | EMAIL : dominic.tarr@gmail.com | URL : http://dominictarr.com
:warning: MODULE : node_modules/jsonify | LICENSE : Public Domain | LICENSE_FILE : node_modules/jsonify/README.markdown | REPOSITORY: https://github.com/substack/jsonify | PUBLISHER : Douglas Crockford | EMAIL : undefined | URL : http://crockford.com/
:warning: MODULE : node_modules/json-schema | LICENSE : AFLv2.1,BSD | LICENSE_FILE : node_modules/json-schema/README.md | REPOSITORY: https://github.com/kriszyp/json-schema | PUBLISHER : Kris Zyp | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/indexof | LICENSE : MIT* | LICENSE_FILE : node_modules/indexof/Readme.md | REPOSITORY: undefined | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/html | LICENSE : BSD* | LICENSE_FILE : node_modules/html/LICENSE | REPOSITORY: https://github.com/maxogden/commonjs-html-prettyprinter | PUBLISHER : Max Ogden | EMAIL : max@maxogden.com | URL : http://maxogden.com
:warning: MODULE : node_modules/glob-to-regexp | LICENSE : BSD* | LICENSE_FILE : node_modules/glob-to-regexp/README.md | REPOSITORY: https://github.com/fitzgen/glob-to-regexp | PUBLISHER : Nick Fitzgerald | EMAIL : fitzgen@gmail.com | URL : undefined
:warning: MODULE : node_modules/uglifyify/node_modules/extend | LICENSE : MIT* | LICENSE_FILE : node_modules/uglifyify/node_modules/extend/LICENSE | REPOSITORY: https://github.com/justmoon/node-extend | PUBLISHER : Stefan Thomas | EMAIL : justmoon@members.fsf.org | URL : http://www.justmoon.net
:warning: MODULE : node_modules/istanbul/node_modules/estraverse | LICENSE : BSD | LICENSE_FILE : node_modules/istanbul/node_modules/estraverse/LICENSE.BSD | REPOSITORY: https://github.com/estools/estraverse | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/domutils | LICENSE : BSD* | LICENSE_FILE : node_modules/domutils/LICENSE | REPOSITORY: https://github.com/FB55/domutils | PUBLISHER : Felix Boehm | EMAIL : me@feedic.com | URL : undefined
:warning: MODULE : node_modules/eslint-plugin-import/node_modules/doctrine | LICENSE : BSD | LICENSE_FILE : node_modules/eslint-plugin-import/node_modules/doctrine/LICENSE.BSD | REPOSITORY: https://github.com/eslint/doctrine | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/css-select | LICENSE : BSD* | LICENSE_FILE : node_modules/css-select/LICENSE | REPOSITORY: https://github.com/fb55/css-select | PUBLISHER : Felix Boehm | EMAIL : me@feedic.com | URL : undefined
:warning: MODULE : node_modules/parse-color/node_modules/color-convert | LICENSE : MIT* | LICENSE_FILE : node_modules/parse-color/node_modules/color-convert/LICENSE | REPOSITORY: https://github.com/harthur/color-convert | PUBLISHER : Heather Arthur | EMAIL : fayearthur@gmail.com | URL : undefined
:warning: MODULE : node_modules/async-foreach | LICENSE : MIT* | LICENSE_FILE : node_modules/async-foreach/LICENSE-MIT | REPOSITORY: https://github.com/cowboy/javascript-sync-async-foreach | PUBLISHER : "Cowboy" Ben Alman | EMAIL : undefined | URL : http://benalman.com/

Generated by :no_entry_sign: dangerJS against 9cf29e2ac68dd6be972460c1ed03d61dc6310ae2

piotr-s-brainhub commented 4 years ago

@aayush420

Thanks for this fix but for me, it doesn't work correctly:

Screenshot 2020-07-18 at 21 50 24 
<Carousel
  centered
  arrows
  addArrowClickHandler
  slidesPerPage={4}
  slidesPerScroll={4}
>
 <img src={imageOne} />
 <img src={imageTwo} />
 <img src={imageThree} />
</Carousel>

Moreover, after pasting this example and clicking the right arrow for the first time, it moves immediately (jumps) with no animation.

aayush420 commented 4 years ago

@aayush420

Thanks for this fix but for me, it doesn't work correctly:

Screenshot 2020-07-18 at 21 50 24 
<Carousel
  centered
  arrows
  addArrowClickHandler
  slidesPerPage={4}
  slidesPerScroll={4}
>
 <img src={imageOne} />
 <img src={imageTwo} />
 <img src={imageThree} />
</Carousel>

Moreover, after pasting this example and clicking the right arrow for the first time, it moves immediately (jumps) with no animation.

Hey @piotr-s-brainhub , The function works with a new prop stickyEdges and you forgot to add it to the JSX part. Also, Ensure that you've enough images to fit the carousel window or else the gap will be shown.

Here's the screenshot after adding the prop to the code shown in your screenshot. DeepinScreenshot_select-area_20200719013123

aayush420 commented 4 years ago

@piotr-s-brainhub I've added a new sidenav entry and a separate example page for the same - stickyEdges under the names Sticky Edges. Here's a screenshot of that: DeepinScreenshot_select-area_20200719014813

RobertHebel commented 4 years ago

@aayush420 The bug @piotr-s-brainhub has mentioned also occurs on v1-legacy branch which means the bug isn't caused by your code. I think we can merge this PR as soon as the issuehunt will be restored for this issue

RobertHebel commented 4 years ago

@aayush420 I've noticed that for this example

<Carousel
  slidesPerPage={2}
  arrows
  stickyEdges
>
  <img src={imageOne} />
  <img src={imageTwo} />
  <img src={imageThree} />
</Carousel>

prop stickyEdges doesn't work

RobertHebel commented 4 years ago

Also, some test would be much appreciated ❤️

aayush420 commented 4 years ago

@aayush420 I've noticed that for this example

<Carousel
  slidesPerPage={2}
  arrows
  stickyEdges
>
  <img src={imageOne} />
  <img src={imageTwo} />
  <img src={imageThree} />
</Carousel>

prop stickyEdges doesn't work

626 Addresses this

aayush420 commented 3 years ago

Knock Knock

RobertHebel commented 3 years ago
Warnings
:warning: MODULE : node_modules/uglify-js/node_modules/wordwrap | LICENSE : MIT* | LICENSE_FILE : node_modules/uglify-js/node_modules/wordwrap/README.markdown | REPOSITORY: https://github.com/substack/node-wordwrap | PUBLISHER : James Halliday | EMAIL : mail@substack.net | URL : http://substack.net
:warning: MODULE : node_modules/vargs | LICENSE : MIT* | LICENSE_FILE : node_modules/vargs/LICENSE | REPOSITORY: undefined | PUBLISHER : Alexis Sellier | EMAIL : self@cloudhead.net | URL : undefined
:warning: MODULE : node_modules/gemini-coverage/node_modules/uglify-js | LICENSE : BSD* | LICENSE_FILE : node_modules/gemini-coverage/node_modules/uglify-js/LICENSE | REPOSITORY: https://github.com/mishoo/UglifyJS2 | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/trim | LICENSE : MIT* | LICENSE_FILE : node_modules/trim/Readme.md | REPOSITORY: undefined | PUBLISHER : TJ Holowaychuk | EMAIL : tj@vision-media.ca | URL : undefined
:warning: MODULE : node_modules/istanbul/node_modules/source-map | LICENSE : BSD | LICENSE_FILE : node_modules/istanbul/node_modules/source-map/LICENSE | REPOSITORY: https://github.com/mozilla/source-map | PUBLISHER : Nick Fitzgerald | EMAIL : nfitzgerald@mozilla.com | URL : undefined
:warning: MODULE : node_modules/gemini-coverage/node_modules/source-map | LICENSE : BSD | LICENSE_FILE : node_modules/gemini-coverage/node_modules/source-map/LICENSE | REPOSITORY: https://github.com/mozilla/source-map | PUBLISHER : Nick Fitzgerald | EMAIL : nfitzgerald@mozilla.com | URL : undefined
:warning: MODULE : node_modules/optimist | LICENSE : MIT* | LICENSE_FILE : node_modules/optimist/LICENSE | REPOSITORY: https://github.com/substack/node-optimist | PUBLISHER : James Halliday | EMAIL : mail@substack.net | URL : http://substack.net
:warning: MODULE : node_modules/map-stream | LICENSE : Custom: https://github.com/dominictarr/event-stream | LICENSE_FILE : node_modules/map-stream/LICENCE | REPOSITORY: https://github.com/dominictarr/map-stream | PUBLISHER : Dominic Tarr | EMAIL : dominic.tarr@gmail.com | URL : http://dominictarr.com
:warning: MODULE : node_modules/jsonify | LICENSE : Public Domain | LICENSE_FILE : node_modules/jsonify/README.markdown | REPOSITORY: https://github.com/substack/jsonify | PUBLISHER : Douglas Crockford | EMAIL : undefined | URL : http://crockford.com/
:warning: MODULE : node_modules/json-schema | LICENSE : AFLv2.1,BSD | LICENSE_FILE : node_modules/json-schema/README.md | REPOSITORY: https://github.com/kriszyp/json-schema | PUBLISHER : Kris Zyp | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/indexof | LICENSE : MIT* | LICENSE_FILE : node_modules/indexof/Readme.md | REPOSITORY: undefined | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/html | LICENSE : BSD* | LICENSE_FILE : node_modules/html/LICENSE | REPOSITORY: https://github.com/maxogden/commonjs-html-prettyprinter | PUBLISHER : Max Ogden | EMAIL : max@maxogden.com | URL : http://maxogden.com
:warning: MODULE : node_modules/glob-to-regexp | LICENSE : BSD* | LICENSE_FILE : node_modules/glob-to-regexp/README.md | REPOSITORY: https://github.com/fitzgen/glob-to-regexp | PUBLISHER : Nick Fitzgerald | EMAIL : fitzgen@gmail.com | URL : undefined
:warning: MODULE : node_modules/uglifyify/node_modules/extend | LICENSE : MIT* | LICENSE_FILE : node_modules/uglifyify/node_modules/extend/LICENSE | REPOSITORY: https://github.com/justmoon/node-extend | PUBLISHER : Stefan Thomas | EMAIL : justmoon@members.fsf.org | URL : http://www.justmoon.net
:warning: MODULE : node_modules/istanbul/node_modules/estraverse | LICENSE : BSD | LICENSE_FILE : node_modules/istanbul/node_modules/estraverse/LICENSE.BSD | REPOSITORY: https://github.com/estools/estraverse | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/domutils | LICENSE : BSD* | LICENSE_FILE : node_modules/domutils/LICENSE | REPOSITORY: https://github.com/FB55/domutils | PUBLISHER : Felix Boehm | EMAIL : me@feedic.com | URL : undefined
:warning: MODULE : node_modules/eslint-plugin-import/node_modules/doctrine | LICENSE : BSD | LICENSE_FILE : node_modules/eslint-plugin-import/node_modules/doctrine/LICENSE.BSD | REPOSITORY: https://github.com/eslint/doctrine | PUBLISHER : undefined | EMAIL : undefined | URL : undefined
:warning: MODULE : node_modules/css-select | LICENSE : BSD* | LICENSE_FILE : node_modules/css-select/LICENSE | REPOSITORY: https://github.com/fb55/css-select | PUBLISHER : Felix Boehm | EMAIL : me@feedic.com | URL : undefined
:warning: MODULE : node_modules/parse-color/node_modules/color-convert | LICENSE : MIT* | LICENSE_FILE : node_modules/parse-color/node_modules/color-convert/LICENSE | REPOSITORY: https://github.com/harthur/color-convert | PUBLISHER : Heather Arthur | EMAIL : fayearthur@gmail.com | URL : undefined
:warning: MODULE : node_modules/async-foreach | LICENSE : MIT* | LICENSE_FILE : node_modules/async-foreach/LICENSE-MIT | REPOSITORY: https://github.com/cowboy/javascript-sync-async-foreach | PUBLISHER : "Cowboy" Ben Alman | EMAIL : undefined | URL : http://benalman.com/

Generated by :no_entry_sign: dangerJS against 13f4465aac414482fb0ae914200d348e0a495795

Lukasz-pluszczewski commented 3 years ago

Hi, sorry for the delay, we have a really busy time. We'll test it and merge as soon as possible. Sorry once again for the inconvenience.

RobertHebel commented 3 years ago

@aayush420 Sorry for the delay. We have been really busy recently. I've checked this PR once again and it seems there is a problem with displaying a couple of last slides with stickyEdges prop

https://user-images.githubusercontent.com/22330154/124141763-f58d8780-da89-11eb-95dd-93043f3d7d04.mov