-
Notifications
You must be signed in to change notification settings - Fork 43
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
Calendar widget #200
Comments
I think we can add Todos to this as well, potentially. |
@Melkeydev I'm utilizing the existing react-calendar library as a launch pad for this work. I only had to add calendar event storage and display below selected weekdays and then of course the condensed view which can be changed in the system settings. It looks like a pretty reputable component. Let me know if this is okay. I've already cut the majority of the fat from the repo itself to make it more maintainable, but the dependencies and build process are still separate from ours. I can combine them and transition from babel->vite to further assimilate into Astrostation. |
@royanger Had a great point - can we just npm install the package instead of trying to re build it? |
Those are fair points. I thought about using their npm scripts, but the dependencies are almost all sub-dependencies, so I wasn't sure how that would impact the final size of the build. Is Vite/Node smart enough to remove/ignore duplicate dependencies during build-time if versions match? What about automated builds in the case anyone decides to change it in the future? Right now, I have to run their scripts directly. I guess that's something I could figure out with Vite to make things work better or at the very least add a note to future developers to manually build with changes present. |
Assuming the package is written correctly, full tree shaking occurs.
I'm not quite sure what you're asking, but if we install from npm there is no build step. The package can immediately be used in the project. I'm not sure what you're referring to with "in the case anyone decides to change it in the future?" but I suspect it is a moot issue if we're using the npm package. |
I think there's a bit of miscommunication. I have not installed from NPM but rather utilized their source code directly with some modifications for the widget. |
There is no miscommunication. I'm fully against that process unless we need to patch the package for some reason. We should not be taking over maintenance of a relatively complicated library via integrated it into our repo when we can simply npm install it. Yes, there are some issues with the npm ecosystem but with tree shaking it should be fine. If we're going to have a calendar widget in the repo, my take would be to build it ourselves to spec. Keep it very lean with only exactly what we need and keep long term maintenance down. |
This is why I am opting to go the route I have specified above. The package react-calendar does not have the full functionality that is desired for this widget. It is missing calendar events and a simplified weekday UI that I have specified is desired for the widget. Ergo, I am suggesting we take what they have now and build on it. That is to say unless you think what they have is good enough for our purposes. Otherwise, building an entirely new widget from scratch (no matter how lean), styling it to fit the app, and maintaining it will require a significant amount of work upfront and potentially in the future (bugs and whatnot). Here are our options as I see it:
@royanger @Melkeydev @mattprivman @creativenull Any input on this? Personally, I'd prefer to do either of the first two, as I see myself practically rebuilding react-calendar to meet our needs. I'll abide by what y'all think is best. |
I think this is the heart of the miscommunication. When the npm option was first brought you mentioned dependencies and not lack of features. Ignoring this library and really any existing solution, can you spec out the needs for the calendar? Let's get that listed and then we can discuss those needs. |
Create a new widget which allows users to:
Also, don't forget to add it to the store so people can add it.
The text was updated successfully, but these errors were encountered: