IvanAdmaers / react-roulette-pro

The best React roulette
https://react-roulette-pro.ivanadmaers.com/
MIT License
63 stars 15 forks source link

Todo #30

Closed IvanAdmaers closed 1 year ago

IvanAdmaers commented 1 year ago

TODO:

To fix the case when the roulette items width and height or one of them is changing in the prizeItemRenderFunction.

Problem: Currently the roulette defines its items width and height from a design plugin.

IvanAdmaers commented 1 year ago

Related to: https://github.com/IvanAdmaers/react-roulette-pro/issues/29

IvanAdmaers commented 1 year ago

Description: the roulette defines its items width and height from a design plugin. So when width and height changes in prizeItemRenderFunction the roulette spinning works incorrect.

Solution: set items width and height in designPlugin

<RoulettePro
...
     designPlugin={() => ({
          prizeItemWidth: 100,
          prizeItemHeight: 100,
          topChildren:
            <div
              className="roulette-pro-regular-design-top horizontal"
            />
          )
      })}
...
/>

Where roulette items width and height comes from: https://github.com/IvanAdmaers/react-roulette-pro/blob/3.0.9/src/components/Roulette/Roulette.tsx#L90

Possibly solution: instead of returning JSX from prizeItemRenderFunction we can return an object with an item width and height.

Now: prizeItemRenderFunction: () => JSX

Idea:

prizeItemRenderFunction: ()  => { JSX: JSX, width: number, height: number }

Important: we should also keep supporting returning only of a JSX element from the prizeItemRenderFunction to not break backward compatibility until next major.

IvanAdmaers commented 1 year ago

Why did I decide to maintain the current behavior? There are three main reasons

  1. If there is a possibility of returning the width and height of prize items from the prizeItemRenderFunction, it could be confusing because developers can return dynamic width or height properties. However, the roulette does not support dynamically changing the width and height of prize items. Setting the width and height of prize items statically in designPlugin.prizeItemWidth and designPlugin.prizeItemHeight helps prevent potential mistakes

  2. Initially, I considered adding the possibility of returning the width and height of prize items from the prizeItemRenderFunction function, such as prizeItemRenderFunction: () => { item: JSX, width: number, height: number }. However, in this case, we would also need to support the previous behavior where the prizeItemRenderFunction function only returns JSX. While implementing this is not difficult, it can be confusing for types, as it requires implementing a union type in TypeScript. I have reservations about using union types since they may confuse new developers who may not understand whether they should return just JSX or an object with JSX, width, and height

  3. I believe that the current behavior with setting the prize items width and height in designPlugin is acceptable.

What did I do? I've added two guards.

  1. If there are prize items with differing width or height, a console error will be displayed.

  2. If the width or height settings of prize items in the prizeItemRenderFunction differ from those in the designPlugin options, a console error will occur. For instance, if you set an item width as 200px in the prizeItemRenderFunction, but in your designPlugin.prizeItemWidth it is set as 300px, you will get a console error.

Both of these guards are almost the same. Why didn't I combine them into a single guard? Alternatively, I could remove the second guard (which checks if any of the items have a different width or height compared to other items, rather than comparing them to the designPlugin settings.) and keep only the first one, which would still be sufficient. I believe this approach is more developer-friendly and enables faster identification and understanding of mistakes.

To reproduce the display of these errors:

<RoulettePro
    ...
    prizeItemRenderFunction={(props) => <div style={{ width: Math.random() }}>123</div>
   ...
/>

In my opinion this issue can be closed until future needs.

IvanAdmaers commented 1 year ago

Released in 3.1.0