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

[Feature] Extend since with abbreviated #311

Merged
merged 1 commit into from Dec 21, 2021

Conversation

Nfinished
Copy link
Contributor

@Nfinished Nfinished commented Dec 21, 2021

This pr extends the return of the since method with a new string prop abbreviated following the pattern 1y2m3d4m5s. I've found myself doing it enough times that I think it's a worthy addition.

I realize that m for month and minute can be ambiguous, but I think most if not all of the time it should be contextually apparent.

abbreviated also deviates from the other properties in that it is always positive regardless of relativity, it's open to discussion but I think it makes sense in context.

I've extended all tests and ensured they pass, and updated the Since type. I didn't update the readme because it already only covers two of the existing returns types.

@Nfinished Nfinished changed the title [Feature] Extend since with abbreviation [Feature] Extend since with abbreviated Dec 21, 2021
@Nfinished
Copy link
Contributor Author

Not sure why builds are failing -- seems environmental?

@Nfinished
Copy link
Contributor Author

Also an open question - spaces or no spaces? No spaces fits the theme better but it's slightly easier to remove spaces than add them.

@spencermountain
Copy link
Owner

oh hey, this is cool. Is this the same as this?

Your scheme, it's human-readable purposes right? Maybe we could do both.
Don't worry about the tests, i've borked master. I'll merge this today, if you're happy with it, or have thoughts on the ISO standard.
Thank you!

@Nfinished
Copy link
Contributor Author

I didn't know that spec existed but it makes sense that it does. Yeah I guess this is some middle ground between spec and readability? If that middle ground is the goal (which I think it should be), maybe we do default to spaces? What do you think?

@Nfinished
Copy link
Contributor Author

Honestly either way someone somewhere's going to be inconvenienced so I'm at a loss.

@spencermountain
Copy link
Owner

yep. i agree. I'll merge it and take a look.
thank you! This is a great idea.

@spencermountain spencermountain changed the base branch from master to dev December 21, 2021 15:36
@spencermountain spencermountain merged commit ae671b2 into spencermountain:dev Dec 21, 2021
@spencermountain
Copy link
Owner

any thoughts on this?

export interface Since {
  diff: Diff
  rounded: string
  qualified: string
  precise: string
  abbreviated: string[]
  iso: string
  direction: 'past' | 'present' | 'future'
}
{   ...,
    precise: '10 hours, 59 minutes ago',
    abbreviated: ['10h', '59m', '10s'],
    iso: 'P0Y0M0DT10H59M10S',
    direction: 'past'
}

the iso format doesn't go negative either - so it's pretty similar to your scheme.
Let me know what you think, shouldn't release this until after the holidays.
cheers

spencermountain added a commit that referenced this pull request Dec 21, 2021
@Nfinished
Copy link
Contributor Author

ha love using an array, very elegant sidestep. Good call on adding directionality too, otherwise it'd have to be inferred from another value. lgtm 👍

@Nfinished
Copy link
Contributor Author

Nfinished commented Dec 21, 2021

Oh what does this mean for 'now'? Is abbreviated an empty array? Is it ['now']? I guess ISO is just all zeroes.

@jecraig
Copy link
Contributor

jecraig commented Dec 21, 2021

I would go with ['0s']. That should keep it consistent in usage and it's still friendly enough for users.

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.

None yet

3 participants