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

Fix multiday events rendering bug #67

Closed
wants to merge 1 commit into from
Closed

Fix multiday events rendering bug #67

wants to merge 1 commit into from

Conversation

antoinerousseau
Copy link
Contributor

Fixes #36.

Inspired from SlavaKvak@a359db3

The main change is that slots and span are now rounded, the same way padding was already rounded. No Math.floor anymore.

@antoinerousseau
Copy link
Contributor Author

I replaced the bug with another: the event is now properly split accross two rows in both en and fr cultures, but its length is wrong in the en culture: it is one day too short.

Test event starts on 2016-04-01 12:00:00 and ends on 2016-04-04 12:00:00

capture d ecran 2016-04-02 15 56 39

capture d ecran 2016-04-02 15 56 54

@antoinerousseau
Copy link
Contributor Author

And @okcompute king of bug is still present, but in en culture only:

capture d ecran 2016-04-02 16 04 57

@antoinerousseau
Copy link
Contributor Author

After some debugging, it seems that the problem comes from the rounding:

capture d ecran 2016-04-02 16 30 02

Math.round might have to replace Math.floor (not sure why these numbers are not already rounded since my interval is between two same times, i.e. 12pm, maybe timezones?)

@antoinerousseau
Copy link
Contributor Author

OK, all good now in both cultures, this fixes both bugs :)

@jquense can you take a look please?

It seems to also resolve the interrogations raised by @KerenChandran in #66 since the end date is included.

antoinerousseau referenced this pull request in SlavaKvak/react-big-calendar Apr 2, 2016
@antoinerousseau antoinerousseau changed the title Fix multiline events rendering bug Fix multiday events rendering bug Apr 2, 2016
@jquense
Copy link
Owner

jquense commented Apr 3, 2016

hi there thanks for the the contribution, I will take a closer look when I get a moment but a cursory glance has me I bit confused by what is going on and why the changes were made. In particular I don't think that math round is the right thing here... could you explain why you are using it? in general "because it made these dates correct" isn't great since there are A LOT of possibilities on a calendar and we need to not break other cases accidentally

@antoinerousseau
Copy link
Contributor Author

Well, I see that the date diff calculation for slots (number of cells in a row) and span (number of cells the event is spanning on the current row) are not giving integers, e.g. 6.95833.. instead of 7, and I don't see why, since the diff method is called with day as the third argument. So it could be understandable for span if the start and end times are different, but what about slots? first and last always have the same time, right, since we are talking about entire days?

@jquense jquense closed this in a2a49c8 Apr 3, 2016
@jquense
Copy link
Owner

jquense commented Apr 3, 2016

As I was diving into the PR here I ended up changing stuff and it ultimately was easier just to commit may changes. Thanks again for the contribution it helped!

@antoinerousseau
Copy link
Contributor Author

Glad I could help! Thanks for the fix :)

You are also welcome to take some of the updates I made to the README, and also please add that missing & to the npm build script

And please publish on NPM :)

@@ -25,7 +25,7 @@
"clean:examples": "rimraf examples/static",
"less": "lessc -x src/less/styles.less ./lib/css/react-big-calendar.css",
"assets": "cpy src/less/* lib/less",
"build": "npm run clean && babel src --out-dir lib && npm run assets & npm run less",
"build": "npm run clean && babel src --out-dir lib && npm run assets && npm run less",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't missing an & the single & is how you run parallel tasks on windows, I assume not so on Mac/*nix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single & is also for parallel tasks on OSX/unix, but I don't think it's a good idea to run these tasks in parallel because if you wait for an exit code from the command, you'll get it before it completes, in addition to have loose output consistency (you don't know when exactly it finishes)...

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

Successfully merging this pull request may close these issues.

2 participants