fix(slider): fixed incorrect update of model and min/max values in same digest cycle by sviams · Pull Request #10980 · angular/material · GitHub

This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Contributor

sviams commented Nov 9, 2017 •

edited by Splaktar

Loading

Implemented check for out of sync state between model and view value in Slider, which occurred when both model and either min and/or max values were updated within the same digest cycle. This is due to the $ngModelCtrl formatters kicking in and validating the view value before the Slider directive $observer on the min and max attributes could fire.

Fixes #9125

googlebot commented Nov 9, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot added the cla: no label

Contributor Author

sviams commented Nov 9, 2017

I signed it!

googlebot commented Nov 9, 2017

CLAs look good, thanks!

googlebot added cla: yes and removed cla: no labels

angular deleted a comment from googlebot

angular deleted a comment from googlebot

Splaktar added needs: review P4: minor type: bug labels

Splaktar self-assigned this

Splaktar self-requested a review

December 19, 2017 16:58

Splaktar added this to the 1.1.6 milestone

Splaktar modified the milestones: 1.1.6, 1.1.7

Splaktar modified the milestones: 1.1.7, 1.1.8

Splaktar modified the milestones: 1.1.8, 1.1.9

Contributor

Splaktar commented Apr 5, 2018

Thank you for your contribution! Please update your commit message to follow the commit message guidelines. This is important for your changes to properly show up in the changelog.

For the last line, Fixes #9125 should be sufficient.

Splaktar suggested changes

View reviewed changes

Contributor

Splaktar left a comment

Functionally, this looks very good. I just had some questions about the last describe block and saw a few missing semicolons. The only other thing was the commit message needing to be updated. When you make the changes, please make sure to squash your commits and then force push your changes to the same branch (sviams:master in this case).

src/components/slider/slider.spec.js Outdated

+                  var slider = null;
+                  beforeEach(function() {
+                    slider = setup('min="0" max="{{max}}" ng-model="model"')

Contributor

Splaktar Apr 5, 2018

Please add semicolon at the end.

src/components/slider/slider.spec.js Outdated

+                  var slider = null;
+                  beforeEach(function() {
+                    slider = setup('min="0" max="{{max}}" ng-model="model"')

Contributor

Splaktar Apr 5, 2018

Please add semicolon at the end.

src/components/slider/slider.spec.js Outdated

+                  var slider = null;
+                  beforeEach(function() {
+                    slider = setup('min="{{min}}" max="10" ng-model="model"')

Contributor

Splaktar Apr 5, 2018

Please add semicolon at the end.

src/components/slider/slider.spec.js Outdated

+                  var slider = null;
+                  beforeEach(function() {
+                    slider = setup('min="{{min}}" max="10" ng-model="model"')

Contributor

Splaktar Apr 5, 2018

Please add semicolon at the end.

src/components/slider/slider.spec.js Outdated

+                    pageScope.$apply();
+                    expect(slider.attr('aria-valuenow')).toEqual('6');
+                    expect(slider.attr('aria-valuemin')).toEqual('5');
+                    pageScope.model = 5;

Contributor

Splaktar Apr 5, 2018

This seems to be checking "when updating min and model value equal to previous min simultaneously" rather than "when updating min and model value above previous min simultaneously". Since the previous min was 5 and the new model value is 5. Am I reading this wrong? Was this intended?

Contributor Author

sviams Apr 5, 2018

I agree, this was not entirely clear so I updated both the title and expectations of the test to better match the intention.

Splaktar changed the title ~~fix(slider): Fixed incorrect update of model and min/max values in same digest cycle (#9125)~~ fix(slider): fixed incorrect update of model and min/max values in same digest cycle

Contributor

Splaktar commented Apr 5, 2018

Oh and one more thing, sorry for taking so long to get to reviewing this PR.

Splaktar added in progress and removed needs: review labels


          fix(slider): out of sync state between model and view value in same d…

aabe743

…igest cycle

Fix out of sync state between model and view value that occurs in Slider when both model
and either min and/or max values are updated within the same digest cycle.
This is due to the $ngModelCtrl formatters kicking in and validating the view value before
the Slider directive $observer on the min and max attributes can fire.

Fixes angular#9125

sviams force-pushed the master branch from 88d02f7 to aabe743 Compare

April 5, 2018 08:40

Splaktar approved these changes

View reviewed changes

Contributor

Splaktar left a comment

LGTM

Splaktar added the pr: merge ready label

Splaktar added pr: lgtm and removed in progress labels

Contributor

Splaktar commented Apr 5, 2018

Thanks for the quick turnaround! Sending to presubmit testing now.

Splaktar assigned jelbourn and unassigned Splaktar

jelbourn assigned tinayuangao and unassigned jelbourn

tinayuangao merged commit 50a1616 into angular:master

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request


          fix(slider): out of sync state between model and view value in same d…

5d5380d

…igest cycle (angular#10980)

Fix out of sync state between model and view value that occurs in Slider when both model
and either min and/or max values are updated within the same digest cycle.
This is due to the $ngModelCtrl formatters kicking in and validating the view value before
the Slider directive $observer on the min and max attributes can fire.

Fixes angular#9125

Splaktar pushed a commit that referenced this pull request


          fix(slider): out of sync state between model and view value in same d…

e746136

…igest cycle (#10980)

Fix out of sync state between model and view value that occurs in Slider when both model
and either min and/or max values are updated within the same digest cycle.
This is due to the $ngModelCtrl formatters kicking in and validating the view value before
the Slider directive $observer on the min and max attributes can fire.

Fixes #9125

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

Labels

cla: yes P4: minor pr: lgtm pr: merge ready type: bug