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

Event label format customization added using moment.js #103

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

Conversation

ssadev
Copy link

@ssadev ssadev commented Feb 22, 2022

Issue #98

@nixypanda
Copy link
Owner

Hey thanks for the PR, I am not maintaining this project actively. But will try to take a look at this on the weekend.

@ssadev
Copy link
Author

ssadev commented Feb 23, 2022

Hey thanks for the PR, I am not maintaining this project actively. But will try to take a look at this on the weekend.

Thank you @sherubthakur. Please try to update this if you can. This is a bit important. I'm trying to use it in my company project. It will be very helpful for me.

@nixypanda
Copy link
Owner

Hey, I took a look at this PR. Here's what we need to do to get this merged.

Approach

I like the idea of customizing how to showcase the labels. But I am not too sure of the approach that we have taken on this PR. I would also like to avoid the additional dependency on moment.js.

I think we can achieve all that if we just allow the end-user to customize how they want to label the dates on the timeline. Right now the timeline accepts an array of strings (i.e. Array<String>). Let's change it to something more flexible like
{ date: Date, label: String }.

Example

Right nor you pass something like this

const VALUES = [
    '2008-06-01',
    '2010-06-01',
    '2013-06-01',
    '2015-03-01',
    '2019-01-01',
    '2019-06-17',
    '2019-08-01',
];

but instead they will have to pass something like the following

const VALUES = [
    { date: new Date(2008, 6, 1), label: "label 1" },
    { date: new Date(2010, 6, 1), label: "label 2" },
    { date: new Date(2013, 6, 1), label: "label 3" },
    { date: new Date(2015, 3, 1), label: "label 4" },
    { date: new Date(2019, 1, 1), label: "label 5" },
    { date: new Date(2019, 6, 17), label: "label 6" },
    { date: new Date(2019, 8, 1), label: "label 7" },
];

Another thing we should do is also accept an array of date objects in addition to this and just do a default formatting on it.
i.e.

const VALUES = [
    new Date(2008, 6, 1),
    new Date(2010, 6, 1),
    new Date(2013, 6, 1),
    new Date(2015, 3, 1),
    new Date(2019, 1, 1),
    new Date(2019, 6, 17),
    new Date(2019, 8, 1),
];

With this, we can offer an interface that offers the flexibility of customization to the end-users without additional fields that we have to take care of or additional libraries.

Now, this is probably more work than you signed up for, so let me know if you are still interested in getting this done.

Contributing

I notice there are some formatting changes that you have done. I like them would prefer them to be applied to the codebase. One problem with formatting related changes is that they make it harder for the reviewer to get to the actual logic. Though this problem is easily fixable by performing all that in a separate commit message. If you do that it would be great

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.

2 participants