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 531 ndeviant master #532

Closed piotr-s-brainhub closed 4 years ago

piotr-s-brainhub commented 4 years ago

Deployed to https://beghp.github.io/gh-pages-rc-10
Fixes #389

- [ ] an issue linked to the PR

piotr-s-brainhub commented 4 years ago

This version loads fewer images initially than the current master.

Screenshot 2020-06-14 at 02 13 19 Screenshot 2020-06-14 at 02 13 28
piotr-s-brainhub commented 4 years ago

@ndeviant

Thanks for this feature. Unfortunately, I noticed a bug: in the Responsive example, there's an infinite loader until I switch slides. All the visible slides should be loaded automatically within a short period of time.

ndeviant commented 4 years ago

Looks like I've did something wrong with pull requests :) I've fixed it and created PR to this branch, but it created a separate PR. https://github.com/brainhubeu/react-carousel/pull/534. How can I use this PR for my further fixes for this feature, if there gonna be any?

piotr-s-brainhub commented 4 years ago

Now everything works (it resolves the problem and doesn't break anything on desktop or mobile).

codeNgamer commented 4 years ago

How long till this is merged? I need this feature. Thank you

piotr-s-brainhub commented 4 years ago

@codeNgamer We need a review from another team member. I hope it will be tomorrow.

codeNgamer commented 4 years ago

@codeNgamer We need a review from another team member. I hope it will be tomorrow.

no worries. i'll just keep an eye out. Thanks!

RobertHebel commented 4 years ago

@ndeviant thanks for the PR. Great job It would be cool if we could have updated readme and docs with information about this feature

ndeviant commented 4 years ago

Thanks. Updated readme and images in docs by this PR: https://github.com/brainhubeu/react-carousel/pull/552

ndeviant commented 4 years ago

Hey guys, what else can I do for the feature to be released? @piotr-s-brainhub @RobertHebel Also I see that unit tests are failing, but it seems the issue with another feature which was merged from master. I believe it's not responsibility of this PR.

piotr-s-brainhub commented 4 years ago

@ndeviant

You need to address all the comments.

It's possible something's wrong with the previous PR but unit tests are still passing on master and other branches.

brainhub-devops 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 f9b76dc791bff66f3b1ca773678ec032d50c0859

ndeviant commented 4 years ago

I thought I've addressed all comments that was at that moment. I've created a PR to fix issue with merge https://github.com/brainhubeu/react-carousel/pull/571. Also could you redeploy branch, so that it use new pictures:

Could we use images that have the same size as the first three ones?

ndeviant commented 4 years ago

Hey guys, how long till this is merged, and what can I do for it? Thank you

piotr-s-brainhub commented 4 years ago

@ndeviant

Thanks. We need @RobertHebel to review it again. I hope it will happen today or tomorrow. I'll also test it again.

piotr-s-brainhub commented 4 years ago

I checked again that lazy loading still works correctly (desktop) and it doesn't break anything (desktop and mobile).

piotr-s-brainhub commented 4 years ago

@ndeviant

This PR is published in 1.19.15.

Thanks for your contribution and I invite you to open PRs for other issues (here the issuehunt list: https://issuehunt.io/r/brainhubeu/react-carousel but please note that some of them can have a PR already opened).