The Wayback Machine - https://web.archive.org/web/20201108165014/https://github.com/ng-bootstrap/ng-bootstrap/pull/3572
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

fix(timepicker): When We Enter 60 in Seconds... are not setting to 0 #3572

Open
wants to merge 1 commit into
base: master
from

Conversation

@gpolychronis
Copy link
Contributor

@gpolychronis gpolychronis commented Jan 31, 2020

Attempt to fix #3546 and #3548.

In this case of binding, origin value (model.second) remains the same and destination value (#seconds.input.value) changes. Angular does not correct the destination value, because it detects no change in origin value.

Attempted to fix by forcing the update of destination value.....

@maxokorokov @benouat
Please review

@gpolychronis gpolychronis requested review from maxokorokov and benouat Jan 31, 2020
@codecov-io
Copy link

@codecov-io codecov-io commented Jan 31, 2020

Codecov Report

Merging #3572 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3572      +/-   ##
==========================================
+ Coverage   91.71%   91.74%   +0.03%     
==========================================
  Files         100      100              
  Lines        2909     2920      +11     
  Branches      537      539       +2     
==========================================
+ Hits         2668     2679      +11     
  Misses        183      183              
  Partials       58       58
Flag Coverage Δ
#e2e 56.57% <90.9%> (+0.12%) ⬆️
#unit 88.56% <100%> (+0.04%) ⬆️
Impacted Files Coverage Δ
src/timepicker/timepicker.module.ts 100% <ø> (ø) ⬆️
src/timepicker/timepicker.ts 98.83% <100%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b429418...9b5e392. Read the comment docs.

Copy link
Member

@benouat benouat left a comment

As discussed offline together, for me this is not an Angular false positive, and it is not related to change detection.
It should be fixed at component level

cc @maxokorokov

@gpolychronis gpolychronis force-pushed the gpolychronis:fix/3546 branch 2 times, most recently from e441343 to 02d42fe Feb 12, 2020
@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented Feb 28, 2020

As discussed:

  • remove "[value]"
    - refactor to avoid code duplication
@gpolychronis gpolychronis force-pushed the gpolychronis:fix/3546 branch 2 times, most recently from 4a8878a to 9b5e392 Mar 2, 2020
@gpolychronis gpolychronis force-pushed the gpolychronis:fix/3546 branch from 9b5e392 to a0110bb Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.