aldabil21 / react-scheduler

React scheduler component based on Material-UI & date-fns
The Unlicense
360 stars 129 forks source link

Top Border adds 2 minutes to all the events #82

Closed FranciscoQuero closed 1 year ago

FranciscoQuero commented 1 year ago

Hi, I know that this is a small issue and I don't even know if you would consider this as something to fix or improve.

I have noticed that the lower the step is, the more that an event is shifted from the starting time. For example, if we consider an extreme case where the step is set to 1 minute, we can notice that the event start time is 2 mins after it is supposed to be:

imagen

After taking a look to the code, I think that it may be caused by the following fragment in the CurrentTimeBar.tsx file:

function calculateTop({ today, startHour, step, minuteHeight }: CurrentTimeBarProps): number {
  const now = new Date();

  const minutesFromTop = differenceInMinutes(now, setHours(today, startHour));
  const topSpace = minutesFromTop * minuteHeight;
  const slotsFromTop = minutesFromTop / step;
  const borderFactor = slotsFromTop + BORDER_HEIGHT;
  const top = topSpace + borderFactor;

  return top;
}

The Border Height is adding 1 minute, and I believe that the division of slotsFromTop is adding one more minute, but I'm not sure.

I would be happy to contribute in case we decide that it's worth fixing it (e.g. adding border only if the step size is greater than 15 mins, just as an example, or calculating the top margin proportionally, maybe using seconds as top margins)

What do you think? Thank you for your time building this incredible library!! I love it. Count on me for any help you need with it.

aldabil21 commented 1 year ago

Hi

PRs are welcome for sure. Perhaps I didn't make such an extreme case test, so I can't comment right now.

However, I believe that the CurrentTimeBar component does not influence the eventItem position. If there is miscalculation in the event top position, it's most likely be in this block: https://github.com/aldabil21/react-scheduler/blob/266f75f5c8b43b64b3e1e6bca724aeee76aa989b/src/lib/components/events/TodayEvents.tsx#L38

I might look into it after I get some sleep :) ... and if you could debug it and find a fix, would be lovely.

Cheers

FranciscoQuero commented 1 year ago

Thank you!! I honestly didn't pay attention to the component. You are right, this doesn't look like the correct place so I'll later take a look too.

There's no rush at all, please don't treat this as a priority.

Thank you again

aldabil21 commented 1 year ago

I tried the extreme case of 1m step. the position calculation is correct, the issue is the "date label" was not changed/re-rendered. In your case, the event is actually 10:02, but the title/label shows 10:00.

I think this is also related to #76

Try the v2.5.11 release, and re-open if you still see the issue

FranciscoQuero commented 1 year ago

Hi, unfortunately, v2.5.11 release did not work, neither v2.5.12.

I forgot to mention that my event is set to 10:00, so the title shows the correct time while its position is wrong.

I discovered that the scheduler is adding 1 minute for every hour after 8 am. For example:

imagen

I tried with the examples in the Readme and it is not happening, so after debugging, I found out that what causes this issue is using MUI CssBaseline. I was able to repdouce it in the following Code Sandbox: https://codesandbox.io/s/standard-forked-f8hhhv?file=/src/App.js

Is this a conflict with CSS? Without investing too much time in this issue, is this something you can think of an easy solution for?

Thank you!

Cheers

FranciscoQuero commented 1 year ago

I think that my app doesn't really need the CssBaseline component, so I think that for now, I'll remove it from it, but it was fun to discover this conflict.

If after reading the issue, you come up with a quick fix, that would be awesome. Otherwise, don't spend time on this. Thank you!

aldabil21 commented 1 year ago

I believe the CssBaseline is important, I personally use it in all my projects that uses Mui.

I will dig more into it.

aldabil21 commented 1 year ago

It seems there were a conflict. Try the next release, feel free to re-open if you still have issue