-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat/season-plugin-option #11
feat/season-plugin-option #11
Conversation
plugin/gatsby-browser.js
Outdated
@@ -40,5 +41,14 @@ export const onInitialClientRender = (_, options) => { | |||
} | |||
}; | |||
|
|||
frame(); | |||
if (season) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do if(season && new Date() ...)
not sure if you're a fan of nested if
statements or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real preference.
I think you can do custom validation, ie. letting folks write:
And then custom verify that the string prefixed with the year is date... I also think these dates should be the defaults if no season is set (making it a breaking change 🤯). |
Dates are hard but to me 12-01 is the 12th of January so maybe the long hand way will work better so folks have to write it out saving confusion. Also agree it should have a default. Breaking change is ok. It can be the first stable version 1.0.0 ... but we should probably add a CHANGELOG.md so it's clear, as well as of course, updating the README.md |
What about your proposal + a flag for repeat every year? That is |
See #12 for changelog |
I'll have to look into it but you need the year in the date sting for it to be valid, and then to be able to run I'd be inclined to leave it and if folks want it next year they can update the date string. |
I think as well using the date string folks could include a time E.g
And the |
plugin/gatsby-browser.js
Outdated
frame(); | ||
if (season) { | ||
if ( | ||
new Date(now).getTime() >= new Date(season.start).getTime() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Aren't season.start already a date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are but they get passed as string
so you can't run getTime()
on the unless the new Date
constructor is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see!
I really want it to start again next year, might look into it a little. |
There's other ways to do this by changing the config to use a Maybe something like this? plugins: [
{
resolve: "@raae/gatsby-plugin-let-it-snow",
options: {
season: {
start: {
day: 1,
month: 12,
},
end: {
day: 31,
month: 12,
},
},
},
},
], Then i guess those values could be used to construct a If you like the above API i can make the change, it's no trouble at all Queen! |
Let's ask Twitter 🎉 |
Let's do it as you proposed and I will try to sneak in a repeat flag at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add the defaults, if you add me to your fork.
plugin/gatsby-node.js
Outdated
@@ -4,5 +4,11 @@ exports.pluginOptionsSchema = ({ Joi }) => { | |||
colors: Joi.array() | |||
.items(Joi.string().default("#ffffff")) | |||
.description("Array of hex color values"), | |||
season: Joi.object() | |||
.keys({ | |||
start: Joi.date(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Add default dates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely stuff!
🎉 This PR is included in version 0.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [0.4.1](v0.4.0...v0.4.1) (2021-12-03) ### Bug Fixes * duration broken by [#11](#11) ([616a01f](616a01f))
start
andend
The only slight snag with this is:
When a user sets the dates via the plugin options it must be a valid date object which includes the year. Naturally, next year this wouldn't start the snow unless the user changes the year in the options