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

Changing timeStamp to timestamp to match ios property name. Fixes #4046 #4627

Closed
wants to merge 1 commit into from
Closed

Conversation

msathis
Copy link

@msathis msathis commented Dec 7, 2015

Fixes #4046

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek and @dmmiller to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Dec 7, 2015
@mkonicek
Copy link
Contributor

mkonicek commented Dec 7, 2015

Thanks for the fix! How did you test this? Does anything depend on this being "timeStamp"?

@dmmiller
Copy link

dmmiller commented Dec 7, 2015

Why did you choose iOS instead of Android as the canonical one? Looking through the code, it looks like it will work either way except in ImageCropper.android which seems to require it be timeStamp.

@msathis
Copy link
Author

msathis commented Dec 8, 2015

As @dmmiller suggested it's a breaking change. Someone with good knowledge about the product should look into this. Sorry for half baked PR :(

@msathis msathis closed this Dec 8, 2015
@christopherdro
Copy link
Contributor

@msathis 👍 You had good intentions I think you have here is fine.
I don't see any other code that would conflict with this change. Going to open this back up.

@dmmiller Could you point me to the file containing ImageCropper.android? I can't seem to find anything like that.

I feel like using timestamp over timeStamp keeps things consistent not only across iOS but other languages in the react-native codebase. Plus, timestamp is considered to be one word so it shouldn't need to be written in camel case. :)

@mkonicek
Copy link
Contributor

mkonicek commented Dec 9, 2015

The ImageCropper is part of the Ads Manager unfortunately. Have to fix it manually..

@mkonicek
Copy link
Contributor

mkonicek commented Dec 9, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/444925209028390/int_phab to review.

@satya164
Copy link
Contributor

Seems the import failed. Let's try again.

@satya164
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/444925209028390/int_phab to review.

@satya164
Copy link
Contributor

ping @mkonicek

@mkonicek
Copy link
Contributor

@christopherdro, @msathis, @dmmiller Looks like the web spec uses timeStamp:
https://developer.mozilla.org/en-US/docs/Web/API/Event/timeStamp
http://www.w3.org/TR/DOM-Level-3-Events/

Should we change the iOS key to timeStamp instead?

@dmmiller
Copy link

(y)

I vote we go with the web. Let's change iOS.

@satya164
Copy link
Contributor

@msathis Can you update the PR?

@msathis
Copy link
Author

msathis commented Dec 31, 2015

@satya164 Yes. I will do it today.

@mkonicek
Copy link
Contributor

Hey @msathis! Thanks for making the pull request, but we are closing it due to inactivity (80 days with no activity) to make sure all pull requests are either being worked on or closed. If you want to get your proposed changes merged, please rebase your branch with master and send a new pull request :)

Based on the comments above, we should change iOS to timeStamp.

@mkonicek
Copy link
Contributor

Update: this has now been fixed by @andreicoman11, it's timeStamp on both platforms.

@andreicoman11
Copy link
Contributor

Minor correction: we went for timestamp instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants