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

Added solar time. #156

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added solar time. #156

wants to merge 2 commits into from

Conversation

camiteca
Copy link

@camiteca camiteca commented Feb 4, 2022

What does this PR do?

Adds the solar time function to the library.

What issues does it solve?

#136

README.md Outdated
### Solar time

```js
SunCalc.getSolarTime(/*Date*/ date, /*Number*/ latitude, /*Number*/ utcOffset)
Copy link

Choose a reason for hiding this comment

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

Based on the original source code, it appears this should use longitude.
Second parameter should be utcOffset and third parameter should be Longitude.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. I've been trying different ways to achieve the same. Missed the readme. Thanks.

var localSolarTime = date.getHours() + timeCorrection / 60 + date.getMinutes() / 60;

var solarDate = new Date(0, 0);
solarDate.setMinutes(+localSolarTime * 60);
Copy link

Choose a reason for hiding this comment

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

Is there a better way to handle the date object when it crosses between days? Using 23:58 as a starting time would be a good test.

Copy link

@thkruz thkruz Jul 27, 2022

Choose a reason for hiding this comment

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

I would recommend switching to a date object as the return.

return new Date(date.getTime() + timeCorrection * 60 * 1000);

That will handle the above issue and allow the user to format the date however they want.

@mourner
Copy link
Owner

mourner commented Jul 31, 2022

Thanks for the PR! Is there a reason to separately introduce this method if you can already get solarNoon from getTimes which is essentially the same thing?

@camiteca
Copy link
Author

Hi @mourner, thank you for your time. The main reason behind the PR is that solarNoon is always the same for a given latitude and longitude (as stated, the sun's highest position). The solar time is a calculation of the passage of time based on the position of the Sun in the sky, which varies with the given date.

If there's already a way to achieve this with the library, please let me know. I struggle a bit with the terms :)

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.

3 participants