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

Add calendar component #76

Closed
wants to merge 6 commits into from
Closed

Conversation

gfeun
Copy link

@gfeun gfeun commented Oct 28, 2021

No description provided.

@meowgorithm
Copy link
Member

This is a good start! If I read correctly, this should be considered a work-in-progress, correct?

@gfeun
Copy link
Author

gfeun commented Oct 28, 2021

Yes, i should have written it explicitly here 🙂, this is WIP

This is linked to this issue where i wrote a bit about it: #8 (comment)

I'd like to tweak weekday handling to support internationalization and it also needs some UI considerations.
I'm not sure where the key mapping handling is to be added. I put it in the example at the moment: https://github.com/charmbracelet/bubbletea/pull/145/files

And I did not take a lot of time to study other component in order to make this one coherent.

@meowgorithm
Copy link
Member

I haven't tested it yet, but the code looks pretty good! I agree with making the weekday naming editable.

There's a keybinding bubble you can use for keybinding definitions (example). I'll make a few other quick comments inline.

calendar/calendar.go Outdated Show resolved Hide resolved
calendar/calendar.go Outdated Show resolved Hide resolved
// Header with 2 character prefix of day names
var s string
for i := 0; i < len(m.Weekdays); i++ {
s += m.Weekdays[i][0:2]
Copy link
Member

@meowgorithm meowgorithm Oct 28, 2021

Choose a reason for hiding this comment

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

Careful with this one as you’ll want to slice the runes rather than the bytes (example).

Also make sure to check that the slice is long enough before you slice to avoid a panic.

Copy link
Member

@meowgorithm meowgorithm Oct 28, 2021

Choose a reason for hiding this comment

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

The other thing to keep in mind is that in some cases slicing the weekday name may not make sense (in Chinese Friday is 星期五, with the important part being the 五). It could make sense to simply read the Model.Weekday value exactly as it's set instead (and just initialize it with abbreviated weekday names) or create a weekday type with more metadata:

type Weekday struct {
    Name string
    Abbreviation string
}

Of course open to your thoughts as well.

Copy link
Author

@gfeun gfeun Oct 28, 2021

Choose a reason for hiding this comment

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

I did not take this into account indeed.

Your suggestion of adding a Weekday struct seems on point, i implemented it and added a default EnglishWeekdays var with the Monday to Sunday + abbreviations.

This will have repercussions on the alignment logic. The 2 runes are for easier handling of week day and corresponding numbers

Mo Tu We Th Fr Sa Su 
             1  2  3 
 4  5  6  7  8  9 10 
11 12 13 14 15 16 17 
18 19 20 21 22 23 24 
25 26 27 28 29 30 31

If we have 1 rune abbreviation, as in your chinese example it will need to be accounted for. Maybe by adding padding relative to the abbreviation length.

Copy link
Member

@meowgorithm meowgorithm Oct 28, 2021

Choose a reason for hiding this comment

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

Nicely done!

And actually, most east-Asian characters (i.e. ) are two cells wide.

五
31

With weekday strings wider/shorter than two cells, you could measure the cell width of weekday numbers (lipgloss.Width(string) int) and then use Lip Gloss to set a width and alignment on each number (setting a width will add padding).

@maaslalani
Copy link
Contributor

@gfeun This is absolutely awesome! I know it's been a while but do you still want more reviews on this? If not, I can probably pick up where you left off. I think it would be a useful addition to Gum for easy date picking.

@gfeun
Copy link
Author

gfeun commented Aug 18, 2022

Hey @maaslalani,

Thanks for awaking this PR, its been so long !

I have some unpushed changes locally that allows displaying multiple months side by side
Let me clean this up tonight and i'll push that, happy to have a review after that 😃

image

@maaslalani
Copy link
Contributor

Oh sweet! Cool, just ping me when you need a review!

@gfeun
Copy link
Author

gfeun commented Aug 18, 2022

Hi again 👋

I have pushed the multi months part and tried to add some more comments to explain the whole thing.
I recorded a demo to get a feel of how it looks / works:
render1660860649057

The demo runs the code available in the companion PR here: https://github.com/charmbracelet/bubbletea/pull/145/files

The code is not so straightforward due to padding, line returns etc. And i feel i may have missed opportunities to make it simpler in some places.

I got no more time to work on this today but for reference, this line smells like it could be optimized by pre-allocating the array once instead of in each View call: https://github.com/charmbracelet/bubbles/pull/76/files#diff-2f6b59027d9f6ec18fb232ae4f569614c7aa43fc4806b5f8586bdf9cda0a296eR91

The calendar is also English centric atm, and i did not test a non-english one so there may be some trouble there too.

Another limitation is that it is currently only working with week start on Monday, which may not suit everyone.

Finally i am not familiar with the other components, best practices, conventions around this projects so don´t hesitate to point me to examples.

@maaslalani
Copy link
Contributor

Thanks so much @gfeun, appreciate all the work!

@skatkov
Copy link

skatkov commented Aug 23, 2022

@gfeun just wanted to say, that I love your work here and the fact that you took inspiration from cal utility.

I know, that this is a work in progress, just wanted to cheap in saying that it would be awesome to have an ability to highlight or mark multiple days in that calendar. Once this will get merged, I might take a stab to implement that. Would be awesome if you consider this feature as future addition in your current implementation.

Thanks.

@knz
Copy link
Contributor

knz commented Aug 24, 2022

This is cool!

@muesli muesli added the enhancement New feature or request label Oct 5, 2022
@bashbunni
Copy link
Member

@meowgorithm are there any styling changes we would want to see before merging?

@meowgorithm
Copy link
Member

meowgorithm commented Jun 23, 2023

Will be a little bit before we can properly re-review this, but thank you @bashbunni for bubbling this up.

@maaslalani
Copy link
Contributor

Hey @gfeun! Thank you so much for the contribution again! ❤️

I think this is now a duplicate of:

I'm cleaning up some old PRs so I will close this PR in favour of #475 and hopefully we can get this DateTime / Calendar component in soon!

@maaslalani maaslalani closed this Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants