Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

fix(text-field): Update character counter to update when value is set. #4663

Merged
merged 3 commits into from
May 4, 2019

Conversation

williamernest
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented May 1, 2019

Codecov Report

Merging #4663 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4663      +/-   ##
==========================================
- Coverage   99.26%   98.96%   -0.31%     
==========================================
  Files         129      129              
  Lines        6294     6294              
  Branches      821      821              
==========================================
- Hits         6248     6229      -19     
- Misses         45       64      +19     
  Partials        1        1
Impacted Files Coverage Δ
packages/mdc-textfield/foundation.ts 100% <100%> (ø) ⬆️
packages/mdc-radio/component.ts 87.23% <0%> (-8.52%) ⬇️
packages/mdc-switch/component.ts 91.22% <0%> (-7.02%) ⬇️
packages/mdc-tabs/tab/component.ts 95% <0%> (-5%) ⬇️
packages/mdc-auto-init/index.ts 95.45% <0%> (-4.55%) ⬇️
packages/mdc-ripple/util.ts 94% <0%> (-4%) ⬇️
packages/mdc-ripple/component.ts 96.15% <0%> (-3.85%) ⬇️
packages/mdc-base/component.ts 96.42% <0%> (-3.58%) ⬇️
packages/mdc-tab/component.ts 95.08% <0%> (-3.28%) ⬇️
packages/mdc-checkbox/component.ts 95.78% <0%> (-1.06%) ⬇️

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 5a52847...ce53e41. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 639 screenshot tests passed for commit 837870f vs. master! 💯🎉

}
this.setCharacterCounter_(value.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the context of this change? any reason for setting this when value is same as native input value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For frameworks that update the value property on the input, there is no way to trigger the character counter to resynchronize with the value. Since the character counter doesn't have any public apis in the component, this is a way to trigger that update.

Also, this line doesn't apply to the Safari comment so it seems like there wasn't an explicit reason to include this line in the value check.

}
this.setCharacterCounter_(value.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test case.

Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

LGTM. Please add / update unit test case.

@mdc-web-bot
Copy link
Collaborator

All 642 screenshot tests passed for commit 6a54c56 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 645 screenshot tests passed for commit 32e97ea vs. master! 💯🎉

@williamernest williamernest force-pushed the fix/text-field/char-counter-value branch from 32e97ea to 965f339 Compare May 3, 2019 21:30
Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

Revert all package-lock.json file changes.

@williamernest williamernest force-pushed the fix/text-field/char-counter-value branch from 965f339 to 2d12096 Compare May 3, 2019 21:44
@williamernest
Copy link
Contributor Author

Reverted

@mdc-web-bot
Copy link
Collaborator

All 645 screenshot tests passed for commit 965f339 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 645 screenshot tests passed for commit 2d12096 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 645 screenshot tests passed for commit ce53e41 vs. master! 💯🎉

@williamernest williamernest merged commit acfbe2d into master May 4, 2019
@williamernest williamernest deleted the fix/text-field/char-counter-value branch May 4, 2019 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants