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

Date Fix #7

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Date Fix #7

merged 1 commit into from
Nov 18, 2019

Conversation

ben182
Copy link
Contributor

@ben182 ben182 commented Mar 17, 2017

Issue #6 fixed

@donatj
Copy link
Owner

donatj commented Mar 28, 2017

Can you explain the issue this fixes with perhaps some example code?

@ben182
Copy link
Contributor Author

ben182 commented Apr 3, 2017

As in Issue #6 explained, I fixed the today bug. When you initalize the calendar to the current month it doesnt hightlight the today item correctly. It highlights the very first of the month each time. This was caused by not correctly referencing to the current date in the each day loop.

@donatj
Copy link
Owner

donatj commented Apr 3, 2017

Oh, I see.

Thank you very much for the explanation as I didn't understand what was the actual complaint in #6.

So you're calling new

new \donatj\SimpleCalendar("April 2017")

where "April 2017" is the current month and year without specifying the day of the month. Hmm…

The ability to specify the day is technically intended as a "feature", but it's kind of ham handedly implemented.

I had not considered that someone wouldn't just omit the parameter for the current month. Ala

new \donatj\SimpleCalendar()

which will highlight the current day correctly.

because if you specify for instance April 22 2017 it picks April 22nd.
screen shot 2017-04-03 at 12 18 24 pm

Current month and highlighted day should be seperate parameters not jammed together. I will accept your PR, but I'm afraid I'm not going to tag a release right now as it's a breaking change.

This evening when I get home from work I will start deving current date as an optional separate parameter so you can still control the "current date".

@ben182
Copy link
Contributor Author

ben182 commented Apr 13, 2017

I thought that the Class can be called like this
new \donatj\SimpleCalendar("April 2017")
because even your Readme explained it so: https://github.com/donatj/SimpleCalendar#sample-usage

@donatj
Copy link
Owner

donatj commented Apr 13, 2017

@ben182 It can, yes.

The issue is when the month and year are the current month and year, it also uses the day of that date to select which day to display. It's a bad behaviour, but also a "feature".

As noted, I should break that into a separate parameter. I just have not had a lot of time of late, but that's coming soon.

@donatj donatj changed the base branch from master to 1.x November 18, 2019 21:31
@donatj donatj merged commit 1c856b4 into donatj:1.x Nov 18, 2019
@donatj
Copy link
Owner

donatj commented Nov 18, 2019

This was merged into the 1.x development branch. The final fix however is a little different, I've added a seperate "Today" date value from the current calendar value, which before were kinda of the same value.

@donatj
Copy link
Owner

donatj commented Jan 11, 2023

@ben182 It's sad on my part but this finally made it into a release - see: https://github.com/donatj/SimpleCalendar/releases/tag/v0.7.0

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