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.

Sign up for GitHub

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

Jump to bottom

feat: add fullWithSeconds datetime format #1720

Merged
merged 5 commits into from
Jan 11, 2022
Merged

feat: add fullWithSeconds datetime format #1720

merged 5 commits into from
Jan 11, 2022

Conversation

andipaetzold
Copy link
Contributor

@andipaetzold andipaetzold commented Jan 7, 2022 •
edited
Loading

Adds a new DateTime format full-with-seconds

This is very similar to the default full format but also displays seconds.
I am not happy with the name, so let me know if you have a better idea.

@vercel
Copy link

vercel bot commented Jan 7, 2022 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/contentful-apps/forma-36/6KozccHMAAy78BjjVSfUcLyeAzJ5
✅ Preview: https://forma-36-git-format-full-with-seconds-contentful-apps.vercel.app

@andipaetzold andipaetzold changed the base branch from master to forma-v4 January 7, 2022 14:08
@andipaetzold andipaetzold self-assigned this Jan 7, 2022
@fabe
Copy link
Member

fabe commented Jan 7, 2022

Maybe we could have use-case-based naming? e.g. precise or exact?

@andipaetzold
Copy link
Contributor Author

Maybe we could have use-case-based naming? e.g. precise or exact?

@fabe That might cause issues if we somewhere want to display milliseconds as well. But I agree that precise sounds better than with-seconds.

In general I love the DateTimeFormat interface where you enable parts with boolean flags. Having something like this in forma with the flags date, time, seconds, weekday would be a great interface and avoid naming issues. But I guess it's too late for discussing this now 😬

bgutsol
bgutsol previously requested changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@bgutsol bgutsol left a comment

Choose a reason for hiding this comment

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

forma-v4 branch was already merged to master. Please change the base to master

@andipaetzold andipaetzold changed the base branch from forma-v4 to master January 10, 2022 10:32
@andipaetzold andipaetzold dismissed bgutsol’s stale review January 10, 2022 10:37

I rebased onto master

bgutsol
bgutsol approved these changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@bgutsol bgutsol left a comment

Choose a reason for hiding this comment

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

LGTM

gui-santos
gui-santos suggested changes Jan 10, 2022
View reviewed changes
Copy link
Contributor

@gui-santos gui-santos left a comment

Choose a reason for hiding this comment

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

Looks good

I will only ask you to make the value camelCase, that's what we've been using for props
(e.g. spacingXl, colorBlue, endIcon, etc)

packages/components/datetime/src/types.ts Outdated Show resolved Hide resolved
@andipaetzold
Copy link
Contributor Author

@bgutsol @gui-santos Both happy with fullWithSeconds or should we go for fullExact as @fabe proposed?

@bgutsol
Copy link
Contributor

bgutsol commented Jan 10, 2022 •
edited
Loading

@bgutsol @gui-santos Both happy with fullWithSeconds or should we go for fullExact as @fabe proposed?

Don't have a strong preference, but fullWithSeconds sounds more clear to me

@gui-santos
Copy link
Contributor

gui-santos commented Jan 10, 2022 •
edited
Loading

@bgutsol @gui-santos Both happy with fullWithSeconds or should we go for fullExact as @fabe proposed?

@andipaetzold
I have no preference tbh 🤷
both sound good to me

@fabe
Copy link
Member

fabe commented Jan 10, 2022

I'd go for fullWithSeconds for now!

@andipaetzold andipaetzold changed the title feat: add "full-with-seconds" datetime format feat: add "fullWSeconds" datetime format Jan 11, 2022
@andipaetzold andipaetzold changed the title feat: add "fullWSeconds" datetime format feat: add fullWithSeconds datetime format Jan 11, 2022
bgutsol
bgutsol reviewed Jan 11, 2022
View reviewed changes
Copy link
Contributor

@bgutsol bgutsol left a comment

Choose a reason for hiding this comment

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

@andipaetzold please, update from the latest master to get ci: check links fixed

@andipaetzold
Copy link
Contributor Author

I'd need an approval in chromatic in order to merge this PR

@gui-santos
Copy link
Contributor

I'd need an approval in chromatic in order to merge this PR

@andipaetzold done
Thanks a lot for this contribution 🙌

@andipaetzold andipaetzold merged commit b475954 into master Jan 11, 2022
@andipaetzold andipaetzold deleted the format-full-with-seconds branch January 11, 2022 14:37
@andipaetzold
Copy link
Contributor Author

Any idea why the released types are not up to date?

https://unpkg.com/@contentful/f36-datetime@4.1.0/dist/types.d.ts

DateFormat is missing the new fullWithSeconds option

@suevalov
Copy link
Contributor

@andipaetzold is it only types or the code is also missing?

@andipaetzold
Copy link
Contributor Author

@suevalov The code itself is there.

(Search for fullWithSeconds in https://unpkg.com/browse/@contentful/f36-datetime@4.1.0/dist/main.js)

@bgutsol
Copy link
Contributor

bgutsol commented Jan 12, 2022

@suevalov The code itself is there.

(Search for fullWithSeconds in https://unpkg.com/browse/@contentful/f36-datetime@4.1.0/dist/main.js)

@andipaetzold we redeployed it, and now it has proper types in v 4.1.1: https://unpkg.com/@contentful/f36-datetime@4.1.1/dist/types.d.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgutsol bgutsol bgutsol approved these changes

@suevalov suevalov suevalov approved these changes

@gui-santos gui-santos gui-santos approved these changes

@denkristoffer denkristoffer Awaiting requested review from denkristoffer

@Lelith Lelith Awaiting requested review from Lelith

@m10l m10l Awaiting requested review from m10l

@mshaaban0 mshaaban0 Awaiting requested review from mshaaban0

@massao massao Awaiting requested review from massao massao was automatically assigned from contentful/team-isotopes

@domarku domarku Awaiting requested review from domarku domarku was automatically assigned from contentful/team-isotopes

Assignees

@andipaetzold andipaetzold

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andipaetzold @fabe @bgutsol @gui-santos @suevalov