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

feat(calendar): add calendar widget #138

Merged
merged 2 commits into from
Apr 26, 2023
Merged

Conversation

sophacles
Copy link
Contributor

A calendar widget that with some display options and styling for individual dates.

This widget displays the month provided to it in the constructor. (see docs and the example for more info).

One thing to note: This pulls in a new dependency, the time crate. Since this adds a dependency, it might be worth putting the widget behind a feature flag - I'm happy to do so if that's desired.

@sophacles sophacles force-pushed the calendar_widget branch 2 times, most recently from b6dde51 to 916a0ba Compare April 15, 2023 18:37
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Looks great! Some minor comments only

examples/calendar.rs Show resolved Hide resolved
src/widgets/calendar.rs Outdated Show resolved Hide resolved
src/widgets/calendar.rs Outdated Show resolved Hide resolved
src/widgets/calendar.rs Outdated Show resolved Hide resolved
Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Changes for header LGTM.

I'm not sure whether the time dependency will be an actual problem for anyone (I'd guess it's probably a fundamental enough crate that probably not). The bigger issue on that idea though is that if you do plan to move to using chrono down the line, perhaps feature flagging on whether to use time or chrono calendars could be worth doing up front. I don't have any strong leanings either way on this. It could be best to just leave this until it's an issue.

I'd lean towards adding the change to calendars::Month in this PR the name just seems more correct and changing the widget names down the line would break more dependents.

Slapping an approve as no big deal if you don't make that change either.

@sophacles
Copy link
Contributor Author

I fixed up the docs to remove the mention of Chrono. That was leftover from when I was building this for a personal project, and I chose to not migrate to chrono there. We can decide how to add support for it if there's a desire for it from users.

About naming, i did the namespaced thing, but went with calendar::Monthly instead of calendar::Month because time::Month is also a type, and those two presumably will be used to gether often (for example, the example).

@sayanarijit
Copy link
Member

Should we put calendar and other similar new-dependency-introducing widgets behind feature flags? @orhun @mindoodoo

@mindoodoo
Copy link
Member

Hmm, what would you name the feature ?

@sayanarijit
Copy link
Member

Hmm, what would you name the feature ?

widget-calendar?

@sophacles
Copy link
Contributor Author

sophacles commented Apr 19, 2023

I put in the machinery for the feature and named it widget-calendar. It's easy to change if something else is decided.

ed: The construct feature_name = ["dep: time"] is newer than MSRV. How would i do this in a way that makes 1.59 happy?

@orhun
Copy link
Member

orhun commented Apr 20, 2023

I didn't understand why you went with dep:time instead of time, why is that required?

@sophacles
Copy link
Contributor Author

why is that required?

Apparently it isn't :). I'm still learning about some of these Cargo directives and I think I misunderstood some of what I read.

Pushed a fixup

@orhun
Copy link
Member

orhun commented Apr 21, 2023

Just tested and it works well. GJ! 🎉

One question though, is it possible to add tests for this?

@sophacles
Copy link
Contributor Author

I can add tests (i think). I'll see what I can get in place over the weekend.

Add `widget-calendar` feature. Dispalys a calendar for the month
provided in the constructor with per-day styling options, and several
display options.

This puts `widgets::calendar` behind the `widget-calendar` feature and
makes the `time` dependency optional unless the feature is enabled.
@sophacles
Copy link
Contributor Author

sophacles commented Apr 22, 2023

I've added some tests for the widget, as well as for the date styler.

One thing to note, I'm not sure if CI will actually run the tests, because I don't know if it builds with default features or does something "smart". I added a feature all-widgets in my latest commit, which currently just specs widget-calendar, but as we add other widgets behind feature flags, I presume it will get more entries. It also allows for a simpler testing with cargo test --features all-widgets

edit - the output of the test runners in CI shows that the calendar tests were not run. I'm not sure how to change what features are enabled for the runner.

@orhun
Copy link
Member

orhun commented Apr 22, 2023

I think you can update Makefile.toml like this for running tests for all-widgets feature:

diff --git a/Makefile.toml b/Makefile.toml
index 8afba0a..db9354f 100644
--- a/Makefile.toml
+++ b/Makefile.toml
@@ -100,11 +100,11 @@ args = [
 ]
 
 [tasks.test-crossterm]
-env = { TUI_FEATURES = "serde,crossterm" }
+env = { TUI_FEATURES = "serde,crossterm,all-widgets" }
 run_task = "test"
 
 [tasks.test-termion]
-env = { TUI_FEATURES = "serde,termion" }
+env = { TUI_FEATURES = "serde,termion,all-widgets" }
 run_task = "test"
 
 [tasks.test]

@sophacles
Copy link
Contributor Author

sophacles commented Apr 22, 2023

Thanks @orhun - that looks like it did the trick, I can see the calendar tests being run now.

@orhun orhun requested review from mindoodoo and sayanarijit April 24, 2023 21:59
Copy link
Member

@mindoodoo mindoodoo left a comment

Choose a reason for hiding this comment

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

This looks awesome man !

@mindoodoo mindoodoo changed the title Add calendar widget feat(calendar): add calendar widget Apr 26, 2023
@orhun orhun merged commit f7ab4f0 into ratatui:main Apr 26, 2023
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.

5 participants