Skip to content
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

Potential improvements for the Carousel #4

Open
L1lith opened this issue Sep 7, 2021 · 1 comment
Open

Potential improvements for the Carousel #4

L1lith opened this issue Sep 7, 2021 · 1 comment

Comments

@L1lith
Copy link

L1lith commented Sep 7, 2021

Just a few ideas for the carousel, I wrote my own custom carousel and I wouldn't mind if you co-opted some of the ideas or implementation details into your library. It's not perfect or anything but here are some ideas

  1. You don't need a dedicated CDBCarouselItem component if you just automatically parse the children as CarouselItems by default (unless say they are a CBDCarouselInner or something like that, you can do this automatically)
  2. Add support to customize the forward and back buttons (I've done a small implementation of this here)

I apologize if it's a little messy, it's a component from a project i had to implement myself because I couldn't find any carousel components I liked on NPM.

import React, { useState } from "react";
import "./carousel.scss";
import hasChildren from "../functions/hasChildren";

export function Carousel(props) {
  // Declare a new state variable, which we'll call "count"
  const [index, setIndex] = useState(0);
  let rotateInterval = null;
  const interval = props.hasOwnProperty("interval")
    ? props.interval
    : 30 * 1000;
  const children = (
    !Array.isArray(props.children) ? [props.children] : props.children
  ).filter(
    (child) =>
      child /* exclude null and undefined and other non-truthy values */
  );
  const goNext = (skipInterval = false) => {
    setIndex(getIndexWrapping(index + 1, children.length));
    if (!skipInterval) setChangeInterval();
  };
  const goBack = () => {
    setIndex(getIndexWrapping(index - 1, children.length));
    setChangeInterval();
  };
  const setChangeInterval = () => {
    if (rotateInterval !== null) {
      clearInterval(rotateInterval);
      //rotateInterval = null; // redundant as it already gets reassigned
    }
    rotateInterval = setInterval(() => {
      console.log("called");
      goNext(true);
    }, interval);
  };
  if (Number.isFinite(interval)) setChangeInterval(interval);
  if (children.length < 1) throw new Error("Expected at least 1 child");
  const buttons = [];
  for (let i = 0; i < children.length; i++) {
    const child = children[i];
    if (child.type === Next) {
      children.splice(i, 1);
      i--;
      buttons.push(
        React.cloneElement(child, {
          goNext,
          key: buttons.length + 1,
        })
      );
    } else if (child.type === Back) {
      children.splice(i, 1);
      i--;
      buttons.push(
        React.cloneElement(child, {
          goBack,
          key: buttons.length + 1,
        })
      );
    }
  }
  return (
    <div className="carousel">
      <span className="content">{children[index]}</span>
      <span className="controls">{buttons}</span>
    </div>
  );
}

function getIndexWrapping(newIndex, total) {
  if (!isFinite(total) || total < 1) throw new Error("Invalid Total");
  while (newIndex < 0) {
    newIndex += total;
  }
  newIndex = newIndex % total;
  return newIndex;
}

export function Next(props) {
  const componentProps = { ...props };
  delete componentProps.children;
  delete componentProps.goNext;
  if (componentProps.hasOwnProperty("className")) {
    componentProps.className =
      typeof componentProps.className == "string"
        ? "next " + componentProps.className
        : "next";
  } else {
    componentProps.className = "next";
  }
  return (
    <button onClick={props.goNext} {...componentProps}>
      {hasChildren(props.children) ? props.children : Next}
    </button>
  );
}

export function Back(props) {
  const componentProps = { ...props };
  delete componentProps.children;
  delete componentProps.goBack;
  if (componentProps.hasOwnProperty("className")) {
    componentProps.className =
      typeof componentProps.className == "string"
        ? "back " + componentProps.className
        : "back";
  } else {
    componentProps.className = "back";
  }
  return (
    <button onClick={props.goBack} {...componentProps}>
      {hasChildren(props.children) ? props.children : Back}
    </button>
  );
}
@Speedwares
Copy link
Member

Thats really cool, Just went through the code and getting a good idea of how it works for you and it's pretty neat 👌. Love the error checks that you are doing there, definitely something I think I need to work on more for cbdreact.

For the points you raised you are right I can definitely remove the CDBcarouselItem and pass the children but the issue I have with doing that is the modularity of the codebase and the experience for users. Actually, they are more components like that on cdbreact that I can pass as children instead. But from looking at other libraries I wanted to make each component do at most very few things so it does not get very cumbersome for the user and also to avoid less experienced users that might be using from passing any tags they want as children which can be the case when does not understand at least to some level what is required to build a carousel. So that was basically the thinking towards that.

For the buttons you are right too, I am currently just using a prop to check which icon to set but allowing the user to also set the icon he/she wants would be nice too, or even allow them to pass children the same way you implemented in yours above. I will think more about this. Thank you!

odb23 added a commit that referenced this issue Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants