Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

[bug fix] Date formatting for ISODate string fails on Android 2.1 stock browser, #2275 #2277

Closed
wants to merge 1 commit into from

Conversation

thiloplanz
Copy link

if "offset" is not specified, it messes up the calculations that follow. Apparently, it does not default to 0 in arithmetic context on some older browsers.

if "offset" is not specified, it messes up the calculations that follow. Apparently, it does not default to 0 in arithmetic context on some older browsers.
@ghost ghost assigned jbdeboer Apr 11, 2013
@jbdeboer
Copy link
Contributor

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
  • PR doesn't introduce new api
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@jbdeboer
Copy link
Contributor

Hi Thilo, thanks for the pull request!

Could you add unit tests to this PR as well as fixing the commit message format.

@thiloplanz
Copy link
Author

I am going to need some pointers for the unit test.

The existing tests do not break with this patch (at least not for Safari on OS X), and presumably they cover this feature (will verify that later).

What we probably want here is a test case that demonstrates the problem on Android 2.1 before this patch was applied (and how the patch fixes it). Unfortunately, I have no idea how to run tests for/on Android. Maybe using the SDK simulator?

@petebacondarwin
Copy link
Contributor

@thiloplanz - If you can get the emulator running on your machine then just point its browser at http://localhost:9876/ when running the unit tests.

@petebacondarwin
Copy link
Contributor

I can confirm that our current filterSpec.js tests fail on this issue for Android 2.1 (emulated) and that they pass with the patch in place.

@petebacondarwin
Copy link
Contributor

I suggest an simpler fix though:

offset = offset || 0;

@thiloplanz
Copy link
Author

Thank you, Pete, for confirming test coverage.

So, how do I go about fixing the commit message? Close this pull request, rebase on top of current master (with the simpler fix), and make a fresh pull request?

petebacondarwin added a commit that referenced this pull request May 7, 2013
In older Android browsers, `undefined` does not act like `0` in some
arithmetic operations. This leads to dates being formatted with `NaN`
strings in the dateFilter because the implementation of the `dateGetter`
function allows offset to be an optional parameter.
The fix is to convert offset to 0 if it is undefined.

Closes #2277, #2275
@petebacondarwin
Copy link
Contributor

Reimplemented and merged at : f046f6f.
Thanks.

@petebacondarwin
Copy link
Contributor

@thiloplanz - the best way is to either rebase or amend the commit and force a push. That way we keep all the comments in one PR.

@thiloplanz thiloplanz deleted the issue2275 branch September 10, 2013 06:34
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.

3 participants