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

ref(ts): Refactor DateTime component #14235

Merged
merged 5 commits into from Aug 7, 2019

Conversation

MeredithAnya
Copy link
Member

No description provided.

@MeredithAnya MeredithAnya requested a review from a team July 31, 2019 21:36
@@ -5,7 +5,16 @@ import _ from 'lodash';

import ConfigStore from 'app/stores/configStore';

class DateTime extends React.Component {
type Props = {
date: any;
Copy link
Member

Choose a reason for hiding this comment

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

Even though the propTypes is any, we should be a bit more specific on the type. It looks like it can be either: 1) string, 2) number (e.g. unix timestamp), or 3) Date object

Copy link
Member

Choose a reason for hiding this comment

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

Let's use moment.MomentInput. I suspect the coercedDate line might be unnecessary and can be deleted as moment can handle string and unix time as input.

@MeredithAnya MeredithAnya force-pushed the typescript/datetime-component branch 2 times, most recently from eaa3e2c to af31c2d Compare August 1, 2019 20:17
@@ -8,15 +8,15 @@ type Options = {
type Device = {
Type: string;
Generation: string;
ANumber: string[];
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes supposed to be in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I just thought I'd add them in here instead of doin a separate pr - can always pull them out if thats better

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a totally separate change right?

Copy link
Member

Choose a reason for hiding this comment

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

It's not related to the component in this PR and is not covered by the description, pulling it out separately is better

Copy link
Member Author

Choose a reason for hiding this comment

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

cool - pulled it out and put it in #14267

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@MeredithAnya MeredithAnya merged commit 2616adb into master Aug 7, 2019
@joshuarli joshuarli deleted the typescript/datetime-component branch August 27, 2019 20:11
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants